Marek is away this week and I would prefer that he takes a look at it. On 05/13/2014 09:32 AM, Tuyen The Nguyen wrote: > Hi all, > > We are trying to make SPNEGO SSO work on gatein tomcat packaging. > > Unfortunately, current spnego sso implementation of gatein only works on > jboss because it use Jboss Negotiation library - a specific library for > jboss. > > So, i provide other spnego implementation for gatein tomcat packaging at > https://github.com/nttuyen/gatein-spnego, this implementation works as > an extension and i tested it on master and 3.7.x branch. > > If there some one can review and have any feedback, i would appreciate. > Btw, is there any idea about where this implementation should be in > gatein codebase? > > Thanks! > > TuyenNT. > > > _______________________________________________ > gatein-dev mailing list > gatein-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/gatein-dev >

Hi Marek, Thank you for your comments and i updated code following your comments. I have some comment inline. Could you review it again On Tue, May 20, 2014 at 1:14 AM, Marek Posolda <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: Hi, Great that you have it working on Tomcat. I have some minor comments: - Names of your classes like SPNEGOFilter, SPNEGOLoginModule etc are same like already existing classes inside gatein-sso. How about renaming them to something like TomcatSPNEGOFilter, TomcatSPNEGOLoginModule etc? I renamed these class to SPNEGOSSOFilter and SPNEGOSSOLoginModule - there are 3 configuration files where is SPNEGOFilter defined and it seems that it's needed just inside extension. So it seems that this whole directory https://github.com/nttuyen/gatein-spnego/tree/master/spnego-jar/src/main/... could be removed? I removed all unused configuration file. Now, spnego implementation is only a jar project. - Very minor - I noticed invalid log class here: https://github.com/nttuyen/gatein-spnego/blob/master/spnego-jar/src/main/... Yes, i replace logger at SPNEGOSSOFilter to org.apache.commons.logging.Log, but SPNEGOSSOLoginModule still use org.exoplatform.services.log.Log because AbstractLoginModule require it. Place where to put it: I am not sure. In current form, it can't be put inside gatein-sso because it has dependencies on some gatein-portal artifacts. It should be possible to remove it if you want to do some more changes in your code and perhaps change instructions (For example use SSOInterceptor like other stuff inside gatein-sso instead of org.exoplatform.web.filter.Filter). So in current state, I would suggest to put it inside some separate directory inside gatein-toolbox or inside some separate directory in gatein-portal. When i try make SPNEGOSSOFilter extends from SSOInterceptor instead of org.exoplatform.web.filter.Filter, i can not config this filter to listen on many url-pattern (now, that filter handler 2 url: /login and /spengosso). Do you have any idea?

Btw, I've tested and this implementation work well on both tomcat and jboss packaging. So, What do you think if we replace spnego implementation with this?

Thanks. TuyenNT. Marek On 13.5.2014 12:17, Bolesław Dawidowicz wrote: > Marek is away this week and I would prefer that he takes a look at it. > > On 05/13/2014 09:32 AM, Tuyen The Nguyen wrote: >> Hi all, >> >> We are trying to make SPNEGO SSO work on gatein tomcat packaging. >> >> Unfortunately, current spnego sso implementation of gatein only works on >> jboss because it use Jboss Negotiation library - a specific library for >> jboss. >> >> So, i provide other spnego implementation for gatein tomcat packaging at >> https://github.com/nttuyen/gatein-spnego, this implementation works as >> an extension and i tested it on master and 3.7.x branch. >> >> If there some one can review and have any feedback, i would appreciate. >> Btw, is there any idea about where this implementation should be in >> gatein codebase? >> >> Thanks! >> >> TuyenNT. >> >> >> _______________________________________________ >> gatein-dev mailing list >> gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> >> https://lists.jboss.org/mailman/listinfo/gatein-dev >> > > > _______________________________________________ gatein-dev mailing list gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> https://lists.jboss.org/mailman/listinfo/gatein-dev

Hi Marek, I have updated spnego implementation follow your comments and i also remove all dependencies on gatein-portal artifacts. i think it's already to put this implementation inside gatein-sso now. Could you review it again before we put it inside gatein-sso? Thank you very much! TuyenNT On Mon, May 26, 2014 at 3:17 PM, Marek Posolda <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: On 21.5.2014 09:55, Tuyen The Nguyen wrote: > Hi Marek, > > Thank you for your comments and i updated code following your > comments. I have some comment inline. > Could you review it again > > > On Tue, May 20, 2014 at 1:14 AM, Marek Posolda > <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: > > Hi, > > Great that you have it working on Tomcat. I have some minor > comments: > - Names of your classes like SPNEGOFilter, SPNEGOLoginModule > etc are > same like already existing classes inside gatein-sso. How > about renaming > them to something like TomcatSPNEGOFilter, > TomcatSPNEGOLoginModule etc? > > > I renamed these class to SPNEGOSSOFilter and SPNEGOSSOLoginModule > > > - there are 3 configuration files where is SPNEGOFilter > defined and it > seems that it's needed just inside extension. So it seems > that this > whole directory > https://github.com/nttuyen/gatein-spnego/tree/master/spnego-jar/src/main/... > could be removed? > > > I removed all unused configuration file. Now, spnego > implementation is only a jar project. > > > - Very minor - I noticed invalid log class here: > https://github.com/nttuyen/gatein-spnego/blob/master/spnego-jar/src/main/... > > > Yes, i replace logger at SPNEGOSSOFilter to > org.apache.commons.logging.Log, but SPNEGOSSOLoginModule still > use org.exoplatform.services.log.Log because AbstractLoginModule > require it. > > > > Place where to put it: I am not sure. In current form, it > can't be put > inside gatein-sso because it has dependencies on some > gatein-portal > artifacts. It should be possible to remove it if you want to > do some > more changes in your code and perhaps change instructions > (For example > use SSOInterceptor like other stuff inside gatein-sso instead of > org.exoplatform.web.filter.Filter). So in current state, I > would suggest > to put it inside some separate directory inside > gatein-toolbox or inside > some separate directory in gatein-portal. > > > When i try make SPNEGOSSOFilter extends from SSOInterceptor > instead of org.exoplatform.web.filter.Filter, i can not config > this filter to listen on many url-pattern (now, that filter > handler 2 url: /login and /spengosso). > Do you have any idea? yes, ATM it just doesn't support many url-pattern :) Feel free to open JIRA in GTNSSO. But anyway, if you really want 2 separate URLs to be mapped to same filter instead of use 2 filters, it should be easy to workaround with map to "/*" and use some code like: if (!requestURI.equals("/login") && !requestURI.equals("/spnegosso")) { chain.doFilter(request, response); return; } Hack, but should work:) > > Btw, I've tested and this implementation work well on both tomcat > and jboss packaging. So, What do you think if we replace spnego > implementation with this? I am not sure about this TBH. Fact is that jboss-negotiation was written before SPNEGO support has been added into Sun JDK (It's available from JDK6 and later AFAIK), so theoretically now it should be fine to rely on functionality provided by JDK. But our customers may use different JDK (for example IBM JDK) on various platforms and it's possible that SPNEGO provided by Java won't work everywhere... So I would still prefer to use jboss-negotiation and old implementation for JBoss. Btv. 2 more things I did not noticed earlier (sorry for that): 1) You are using sun specific Base64Encoder and Base64Decoder. I am quite sure that this won't work on all platforms (like IBM JDK). We have org.gatein.common.util.Base64 in gatein-common. How about switching to use this one instead? 2) Right now, you are doing SPNEGO server login in filter constructor so it's cached if I understand correctly. I think it's not good to cache the LoginContext of the server login. For example if credential of the server principal and keytabs change or if Kerberos server is unavailable, you will still have cached instance of server subject. IMO server subject should be re-logged during each login process. I am sorry for late response :/ Marek > Thanks. > > TuyenNT. > > > > Marek > > > On 13.5.2014 12:17, Bolesław Dawidowicz wrote: > > Marek is away this week and I would prefer that he takes a > look at it. > > > > On 05/13/2014 09:32 AM, Tuyen The Nguyen wrote: > >> Hi all, > >> > >> We are trying to make SPNEGO SSO work on gatein tomcat > packaging. > >> > >> Unfortunately, current spnego sso implementation of gatein > only works on > >> jboss because it use Jboss Negotiation library - a > specific library for > >> jboss. > >> > >> So, i provide other spnego implementation for gatein > tomcat packaging at > >> https://github.com/nttuyen/gatein-spnego, this > implementation works as > >> an extension and i tested it on master and 3.7.x branch. > >> > >> If there some one can review and have any feedback, i > would appreciate. > >> Btw, is there any idea about where this implementation > should be in > >> gatein codebase? > >> > >> Thanks! > >> > >> TuyenNT. > >> > >> > >> _______________________________________________ > >> gatein-dev mailing list > >> gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> > >> https://lists.jboss.org/mailman/listinfo/gatein-dev > >> > > > > > > > > _______________________________________________ > gatein-dev mailing list > gatein-dev(a)lists.jboss.org <mailto:gatein-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/gatein-dev > >

Hi Marek, I created an issue and PR for this implementation at: https://issues.jboss.org/browse/GTNSSO-29 and https://github.com/gatein/gatein-sso/pull/4 Please, review and merge it if it's ok. Thank you so much! TuyenNT. On Fri, May 30, 2014 at 5:53 PM, Marek Posolda <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: Great! Feel free to send PR to gatein-sso. Probably add new module with your implementation as top level. Or maybe we can do spnego-parent and keep existing implementation in "spnego-parent/spnego" directory inside it and your in "spnego-parent/gatein-spnego" . Just a note that for backwards compatibility, I would like to keep current spnego to have same groupId and artifactId as it's still has. Marek On 29.5.2014 04:23, Tuyen The Nguyen wrote: > Hi Marek, > > I have updated spnego implementation follow your comments and i > also remove all dependencies on gatein-portal artifacts. > i think it's already to put this implementation inside gatein-sso > now. > > Could you review it again before we put it inside gatein-sso? > > Thank you very much! > > TuyenNT > > > > On Mon, May 26, 2014 at 3:17 PM, Marek Posolda > <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: > > On 21.5.2014 09:55, Tuyen The Nguyen wrote: >> Hi Marek, >> >> Thank you for your comments and i updated code following >> your comments. I have some comment inline. >> Could you review it again >> >> >> On Tue, May 20, 2014 at 1:14 AM, Marek Posolda >> <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: >> >> Hi, >> >> Great that you have it working on Tomcat. I have some >> minor comments: >> - Names of your classes like SPNEGOFilter, >> SPNEGOLoginModule etc are >> same like already existing classes inside gatein-sso. >> How about renaming >> them to something like TomcatSPNEGOFilter, >> TomcatSPNEGOLoginModule etc? >> >> >> I renamed these class to SPNEGOSSOFilter and >> SPNEGOSSOLoginModule >> >> >> - there are 3 configuration files where is SPNEGOFilter >> defined and it >> seems that it's needed just inside extension. So it >> seems that this >> whole directory >> https://github.com/nttuyen/gatein-spnego/tree/master/spnego-jar/src/main/... >> could be removed? >> >> >> I removed all unused configuration file. Now, spnego >> implementation is only a jar project. >> >> >> - Very minor - I noticed invalid log class here: >> https://github.com/nttuyen/gatein-spnego/blob/master/spnego-jar/src/main/... >> >> >> Yes, i replace logger at SPNEGOSSOFilter to >> org.apache.commons.logging.Log, but SPNEGOSSOLoginModule >> still use org.exoplatform.services.log.Log >> because AbstractLoginModule require it. >> >> >> >> Place where to put it: I am not sure. In current form, >> it can't be put >> inside gatein-sso because it has dependencies on some >> gatein-portal >> artifacts. It should be possible to remove it if you >> want to do some >> more changes in your code and perhaps change >> instructions (For example >> use SSOInterceptor like other stuff inside gatein-sso >> instead of >> org.exoplatform.web.filter.Filter). So in current state, >> I would suggest >> to put it inside some separate directory inside >> gatein-toolbox or inside >> some separate directory in gatein-portal. >> >> >> When i try make SPNEGOSSOFilter extends from SSOInterceptor >> instead of org.exoplatform.web.filter.Filter, i can not >> config this filter to listen on many url-pattern (now, that >> filter handler 2 url: /login and /spengosso). >> Do you have any idea? > yes, ATM it just doesn't support many url-pattern :) Feel > free to open JIRA in GTNSSO. But anyway, if you really want 2 > separate URLs to be mapped to same filter instead of use 2 > filters, it should be easy to workaround with map to "/*" and > use some code like: > > if (!requestURI.equals("/login") && > !requestURI.equals("/spnegosso")) { > chain.doFilter(request, response); > return; > } > > Hack, but should work:) > > >> >> Btw, I've tested and this implementation work well on both >> tomcat and jboss packaging. So, What do you think if we >> replace spnego implementation with this? > I am not sure about this TBH. Fact is that jboss-negotiation > was written before SPNEGO support has been added into Sun JDK > (It's available from JDK6 and later AFAIK), so theoretically > now it should be fine to rely on functionality provided by > JDK. But our customers may use different JDK (for example IBM > JDK) on various platforms and it's possible that SPNEGO > provided by Java won't work everywhere... So I would still > prefer to use jboss-negotiation and old implementation for JBoss. > > Btv. 2 more things I did not noticed earlier (sorry for that): > > 1) You are using sun specific Base64Encoder and > Base64Decoder. I am quite sure that this won't work on all > platforms (like IBM JDK). We have > org.gatein.common.util.Base64 in gatein-common. How about > switching to use this one instead? > > 2) Right now, you are doing SPNEGO server login in filter > constructor so it's cached if I understand correctly. I think > it's not good to cache the LoginContext of the server login. > For example if credential of the server principal and keytabs > change or if Kerberos server is unavailable, you will still > have cached instance of server subject. IMO server subject > should be re-logged during each login process. > > I am sorry for late response :/ > Marek > >> Thanks. >> >> TuyenNT. >> >> >> >> Marek >> >> >> On 13.5.2014 12:17, Bolesław Dawidowicz wrote: >> > Marek is away this week and I would prefer that he >> takes a look at it. >> > >> > On 05/13/2014 09:32 AM, Tuyen The Nguyen wrote: >> >> Hi all, >> >> >> >> We are trying to make SPNEGO SSO work on gatein >> tomcat packaging. >> >> >> >> Unfortunately, current spnego sso implementation of >> gatein only works on >> >> jboss because it use Jboss Negotiation library - a >> specific library for >> >> jboss. >> >> >> >> So, i provide other spnego implementation for gatein >> tomcat packaging at >> >> https://github.com/nttuyen/gatein-spnego, this >> implementation works as >> >> an extension and i tested it on master and 3.7.x branch. >> >> >> >> If there some one can review and have any feedback, i >> would appreciate. >> >> Btw, is there any idea about where this >> implementation should be in >> >> gatein codebase? >> >> >> >> Thanks! >> >> >> >> TuyenNT. >> >> >> >> >> >> _______________________________________________ >> >> gatein-dev mailing list >> >> gatein-dev(a)lists.jboss.org >> <mailto:gatein-dev@lists.jboss.org> >> >> https://lists.jboss.org/mailman/listinfo/gatein-dev >> >> >> > >> > >> > >> >> _______________________________________________ >> gatein-dev mailing list >> gatein-dev(a)lists.jboss.org >> <mailto:gatein-dev@lists.jboss.org> >> https://lists.jboss.org/mailman/listinfo/gatein-dev >> >> > >