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

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&gt; 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&gt; > 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 >> >> https://lists.jboss.org/mailman/listinfo/gatein-dev >> >> >> > >> > >> > >> >> _______________________________________________ >> gatein-dev mailing list >> gatein-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/gatein-dev >> > > >