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

Unclear/incorrect understanding of mutual auth and context continuation #39

Open
michael-o opened this issue Aug 12, 2021 · 7 comments
Open
Labels

Comments

@michael-o
Copy link
Contributor

michael-o commented Aug 12, 2021

I am quite confused by the handling of mutual authentication and context continuation/completion. I don't take this as a solid argument because it does not mean that this module should not implement it correctly (regardless of TLS). mod_auth_gssapi perfectly works. My SpnegoAuthenticator fully respects it. I would expect this module to solely rely on the context continuation flag instead of the mutual flag to complete the context. I don't see any checks for continuation. In fact, even if mutual is disabled the server still sends a small token:
Modified curl:

$ git diff
diff --git a/lib/curl_gssapi.c b/lib/curl_gssapi.c
index 5810dad14..1b9be62f3 100644
--- a/lib/curl_gssapi.c
+++ b/lib/curl_gssapi.c
@@ -51,9 +51,6 @@ OM_uint32 Curl_gss_init_sec_context(
 {
   OM_uint32 req_flags = GSS_C_REPLAY_FLAG;

-  if(mutual_auth)
-    req_flags |= GSS_C_MUTUAL_FLAG;
-
   if(data->set.gssapi_delegation & CURLGSSAPI_DELEGATION_POLICY_FLAG) {
 #ifdef GSS_C_DELEG_POLICY_FLAG
     req_flags |= GSS_C_DELEG_POLICY_FLAG;

Against mod_auth_gssapi:

$ LD_LIBRARY_PATH=/tmp/curl/lib  /tmp/curl/bin/curl   -X HEAD --verbose  https://deblndw011x.ad001.siemens.net/repos/websvn/ -k --negotiate -u :
* Server auth using Negotiate with user ''
> HEAD /repos/websvn/ HTTP/1.1
> Host: deblndw011x.ad001.siemens.net
> Authorization: Negotiate YIIMjQYGKwYBBQUCoIIMgTCCDH2gDTALBgkqh...
> User-Agent: curl/7.79.0-DEV
> Accept: */*
>
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* old SSL session ID is stale, removing
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Thu, 12 Aug 2021 09:28:54 GMT
< Server: Apache
* Negotiate: noauthpersist -> 0, header part: true
< Persistent-Auth: true
< WWW-Authenticate: Negotiate oRQwEqADCgEAoQsGCSqGSIb3EgECAg==
< X-Frame-Options: SAMEORIGIN
< X-Powered-By: PHP/7.4.21
< Content-Language: de
< Content-Type: text/html; charset=UTF-8
* no chunk, no close, no size. Assume close to signal end
<

same with vanilla curl:

$ curl   -X HEAD --verbose  https://deblndw011x.ad001.siemens.net/repos/websvn/ -k --negotiate -u :
* Server auth using Negotiate with user ''
> HEAD /repos/websvn/ HTTP/1.1
> Host: deblndw011x.ad001.siemens.net
> Authorization: Negotiate YIIMjQYGKwYBBQUCoIIM...
> User-Agent: curl/7.78.0
> Accept: */*
>
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* old SSL session ID is stale, removing
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Thu, 12 Aug 2021 09:30:19 GMT
< Server: Apache
* Negotiate: noauthpersist -> 0, header part: true
< Persistent-Auth: true
< WWW-Authenticate: Negotiate oYG3MIG0oAMKAQChCwYJKoZIhvcSAQICooGfBIGcYIGZBgkqhkiG9xIBAgICAG+BiTCBhqADAgEFoQMCAQ+iejB4oAMCARKicQRvvNAqhdgnvC0LXYN19pJxezVYDUN173D2KCPxe6mAIGDQdBFkh+4I9DNSyMlQ1UIXDHMUPu9VK931/lOSLpusjdu/mS42RWE95kp5uPxWhaQT6UmS1pTNLIB+rf78M3CE2oovKAD0TQyEb+3xrNji
< X-Frame-Options: SAMEORIGIN
< X-Powered-By: PHP/7.4.21
< Content-Language: de
< Content-Type: text/html; charset=UTF-8
* no chunk, no close, no size. Assume close to signal end
<

I am willing to provide a PR which uses this property, ironically written with your participation.

Checked also the source code of gss-client/gss-server they both use the continuation flag to complete a context based on RFC 7546.

@pythongssapi pythongssapi locked as too heated and limited conversation to collaborators Aug 12, 2021
@pythongssapi pythongssapi unlocked this conversation Aug 17, 2021
@stale stale bot added the stale label Sep 14, 2021
@stale stale bot closed this as completed Sep 28, 2021
@michael-o
Copy link
Contributor Author

@jborean93 Can you please reopen this one. I'd like to discuss a possible PR for this.

@jborean93 jborean93 reopened this Jan 16, 2022
@jborean93 jborean93 added question and removed stale labels Jan 16, 2022
@jborean93
Copy link
Contributor

Opened, keep in mind this is another "feature" that was migrated as is from requests-kerberos where mutual auth did have a valid use case (the context could have been used to wrap/unwrap data based on the security context). In fact it looks like frozencemetery changed the default to DISABLED 498da2e. IIRC this is because mutual auth over HTTP is mostly useless as the messages aren't being wrapped or signed by the security context and HTTPS offers it's own server authentication + encryption.

Happy to hear your thoughts on the matter but from what I can gather there is little benefit to re-enabling it.

@michael-o
Copy link
Contributor Author

Thanks, I will get back to this next week.

@michael-o
Copy link
Contributor Author

So here is my understanding based von RFC 7546 which I don't consider being implemented properly:

C: Send request
S: Respond with 401, Negotiate
C: Create security context, set flags, generate token; consume 401 response body silently; resend request with token; evaluate whether the context needs to be continued and persist context
S: Evaluate token and check whether context is complete and whether output token needs to be send
C: Status is 401, authentication failed; dispose context, handle off error/status to user; status is non-401 check context continuation and get token from server response; if no token, log a warning, dispose context and handle off request to user, token validated, context complete; log debug success message (for debugging purposes); if context is not complete, generate new token; consume response, resend request with new token; continue loop until done

Remarks:

  • The output token can be on any status code except 401 from the server because the server will not evaluate your request until you haven't authenticated; in our APIs we surely return 30x, 403, 404, etc, and 50x with the output token since authentication was successful after all.
  • The context completion is completely decoupled from requested context flags like conf, wrapping or mutual authentication. See also: brandond/requests-negotiate-sspi@37a1347
  • Disabling mutual auth does not imply that the context does not have to be completed. I have tested this with MIT Kerberos, JGSS, and SSPI, they all return a small token anyway. I see little reason to disable it since it comes for free and has a minimal overhead. I cannot expect everyone to use TLS, may in private networks this is just not necessary, but you still want to securely complete the loop.

Note that context flags aren't requirements, they are indications/hints and the caller must verify whether the mech and peer have this enabled otherwise you need to fail. Cyrus SASL code is a good read on that.

Let me know what you think. This can drastically simplify the code. I'd like to take a shot on the changes if we can come to an agreement.

BTW: With the never completed context I was able to find a bug in my JGSS-based setup and only noticed it with the SSPI module, not this one or curl: curl/curl#5235

@jborean93
Copy link
Contributor

Just having a read over what you've posted and I'm not against having the code process the token on return regardless of the mutual auth setting. I'm not sure how much complexity it will remove, it seems like it would mostly just be removing

if self.mutual_authentication not in (REQUIRED, OPTIONAL):
log.debug("handle_other(): returning {0}".format(response))
return response
with a few other tweaks in that function.

That is unless you are suggesting to change the entire workflow so that it will continually re-send the auth tokens until it is complete. This does have the added benefit of supporting auth tokens that require multiple round trips (like NTLM).

Just as an FYI that short little message you receive regardless from some of your servers is just the SPNEGO completion token

# python -m spnego --format yaml --token oRQwEqADCgEAoQsGCSqGSIb3EgECAg==
MessageType: SPNEGO NegTokenResp
Data:
  negState: accept-complete (0)
  supportedMech: Kerberos (1.2.840.113554.1.2.2)
  responseToken:
  mechListMIC:
RawData: A1143012A0030A0100A10B06092A864886F712010202

But considering the token is there and we don't need to do extra work to get it there's no reason why it shouldn't just be processed for completions sake.

@github-actions
Copy link

This issue is stale because it has been open for 4 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.

@github-actions github-actions bot added the stale label Mar 16, 2022
@jborean93 jborean93 removed the stale label Mar 16, 2022
@michael-o
Copy link
Contributor Author

Don't close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants