Re: [gatein-dev] SPNEGO SSO implementation for gatein tomcat packaging

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 >> >> > >