Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpcserver: x509_login: Handle unsuccessful certificate login gracefully #560

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Mar 9, 2017

When mod_lookup_identity is unable to match user by certificate (and username)
it unsets http request's user. mod_auth_gssapi is then unable to get Kerberos
ticket and doesn't set KRB5CCNAME environment variable.
x509_login.call now returns 401 in such case to indicate that request was
not authenticated.

https://pagure.io/freeipa/issue/6225

def __call__(self, environ, start_response):
self.debug('WSGI login_x509.__call__:')

try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this operation called always, so from performance POW it should be faster. Please use:

if 'KRB5CCNAME' not in environ:
     return ...

Exception handling is expensive.
But we are talking about miliseconds so I'm fine with this too

When mod_lookup_identity is unable to match user by certificate (and username)
it unsets http request's user. mod_auth_gssapi is then unable to get Kerberos
ticket and doesn't set KRB5CCNAME environment variable.
x509_login.__call__ now returns 401 in such case to indicate that request was
not authenticated.

https://pagure.io/freeipa/issue/6225
@flo-renaud flo-renaud added the ack Pull Request approved, can be merged label Mar 15, 2017
@flo-renaud
Copy link
Contributor

Hi,
the invalid cert login correctly returns 401.

@MartinBasti
Copy link
Contributor

master:

  • 70889d4 rpcserver: x509_login: Handle unsuccessful certificate login gracefully

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants