Looks good to me.

Why is this necessary? I only see this method used by the krbLogin() method, and it's specifically passed a principal.

I agree with @mikeb here

This is indeed used in the krb case, so that I can still use a username even if krb is used in an installation.
In our case, this can be used by for example the modularity build system and sigul signing server, which only know a users' username and not their entire krb5 principal name.
Also, building the krb5 principal name does not have to be consistent: for example, we have puiterwijk@FEDORAPROJECT.ORG and robosignatory/autosign01.phx2.fedoraproject.org@FEDORAPROJECT.ORG.

With this patch, sigul and modularity could just pass proxyuser=puiterwijk or proxyuser=autopen (since neither use krb auth directly they do not have the users' full principal).

I don't think this patch achieves what you want it to. I think you want to be able to use the proxy user feature when logging in via the new GSSAPI auth.

Firstly, GSSAPI auth doesn't call the krbLogin method at all, it calls sslLogin() (we can discuss whether this is a good idea separately).

The way the proxy user feature works in sslLogin() is that the DN of the user who should be allowed to act as any other user is put into ProxyDNs on the hub. When someone logs in to the hub using a client certificate, the DN from that certificate is checked against the list of ProxyDNs. If the client DN is in the list, the user is allowed to become another user, specified by the proxyuser argument.

When a user that's logging in via GSSAPI calls sslLogin(), the client certificate DN is set to be the value of REMOTE_USER, which is just their username. That value is then checked against the ProxyDNs list.

So, if you want a GSSAPI user to be able to log in to Koji as another user, I believe you should be able to put their username into ProxyDNs, separated by |, and it will work.

Note that there is now increased possibility of collisions between client certificate names and GSSAPI principal names. sslLogin() should probably be redesigned to check the full GSSAPI principal (including DOMAIN, does mod_auth_gssapi expose this anywhere?) against the ProxyDNs list (which should probably be renamed ProxyUsers).

Ok, I see what you are getting at here, but I think this is the wrong way to do it. The logic should be explicit, rather than sneaking a plain username into a variable named login_principal and teaching a function named getUserIdFromKerberos to deal with it.

This might not be workable. If krbLogin passes a bare username for the proxyuser arg, then the LoginCreatesUser option will do the wrong thing (that is, create a new user with the wrong krb_principal value).

@mikem The issue is that, with the current GSSAPI implementation, krbLogin() is never called. The gssapi_login() method in ClientSession calls sslLogin(), which is sent via the /kojihub/ssllogin URL (specified in _prepCall()). I assume in the Fedora setup that /kojihub/ssllogin is now configured to use mod_auth_gssapi auth, and that it sets REMOTE_USER, which is what the sslLogin() method is using as the authenticated username.

This all seems a bit hacky. The current approach makes it impossible to use both SSL and GSSAPI auth at the same time (which may be desirable for a number of reasons). gssapi_login() should be calling a separate gssapiLogin() method, which would be accessed via a /kojihub/gssapilogin URL, which would be configured to use mod_auth_gssapi.

I think the big pain point here is that ClientSession.krb_login can use either krbLogin or sslLogin depending options and server capabilities, so its proxyuser option can go to either place.

I agree that, in retrospect, some parts of the implementation should probably have been done differently.

Granted, since there is a separate (client) call and authtype for gssapi, I'm not 100% sure the value of the krb_login trying gssapi.

Agreed, maybe we should just take that out. I don't love any code followed by an except: pass.

@mikem The issue is that, with the current GSSAPI implementation, krbLogin() is never called. The gssapi_login() method in ClientSession calls sslLogin(), which is sent via the /kojihub/ssllogin URL (specified in _prepCall()). I assume in the Fedora setup that /kojihub/ssllogin is now configured to use mod_auth_gssapi auth, and that it sets REMOTE_USER, which is what the sslLogin() method is using as the authenticated username.
This all seems a bit hacky. The current approach makes it impossible to use both SSL and GSSAPI auth at the same time (which may be desirable for a number of reasons). gssapi_login() should be calling a separate gssapiLogin() method, which would be accessed via a /kojihub/gssapilogin URL, which would be configured to use mod_auth_gssapi.

It does not make this impossible, and we have had that for over a month until we disabled SSL login: I just configured Apache to require either a client cert or a valid-user, and told GSSAPI to return LocalNameOnly.

The problem here is with clients that are authenticated via krbV (our builders, and modularity/sigul), have to pass the full proxyUser's credential, which with SSL auth wasn't needed.

GSSAPI is NOT in play for the goal of this PR. Even better: when sigul/modularity would be using GSSAPI auth, this entire difference is no issue.

It's purely the different in behaviour of proxyuser= between ssl_login and krb_login.

I would be okay with having an additional proxyUserIsNotPrincipalButUsername option to krb_login, but I'm not sure you want that either.

builders shouldn't need to pass proxyuser at all. Can sigul/modularity be configured to use GSSAPI auth instead of krbV?

builders shouldn't need to pass proxyuser at all. Can sigul/modularity be configured to use GSSAPI auth instead of krbV?

Sure. I just thought I'd submit this upstream since it might confuse other people as well if they ever switch.

ping, can this please be reopened and looked at. upgrading to 1.12 cause sigul to be unable to work at the least. We need to ensure that we can sign packages, and do other tasks like login on the web interface that use proxyuser

Reopened as #408.