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

fix: properly map SSPI flags to Kerberos GSSAPI flags #189

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

irvingoujAtDevolution
Copy link
Contributor

Note: This change might be breaking. Confidentiality and integrity is not implied anymore. User needs to actively specify these two flag to get sign and seal.

This change is critical for LDAPs, as Active Directory does not accept Kerberos with sign and seal flags.

This implementation requires reviewers attention, in particular, at line 408 of src/kerberos/client/generators.rs. Where windows never specified how these flags map to the GSS checksum flags. This is purely guessed from their Kerberos documentation at section 3.2.5.2. and the SSPI documentation .

Whether this implementation is correct or not, we need a way for caller to change that flag.

@irvingoujAtDevolution irvingoujAtDevolution changed the title Fix: the GSS Flags were hard coded, which does not work for Ldaps,Fixed. Fix: the GSS Flags were hard coded, which does not work for Ldap Dec 12, 2023
@awakecoding
Copy link
Contributor

It looks good to me, I'm surprised it has worked so far. We'll need to test that it doesn't break things for other protocols though

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Thank you for that! I left a few comments for improvements, but I didn’t spot any significant issue just by looking at the code in isolation. Let’s test this more thoroughly (via FreeRDP, IronRDP, etc) once merged.
Feel free to address the comments (or not), and merge yourself.

PS: I’ll rename the PR title to better reflect the changes, and keep consistency with other commits (although casing for commit type is not imposed by conventional commit specification).

src/kerberos/client/generators.rs Outdated Show resolved Hide resolved
src/kerberos/client/generators.rs Show resolved Hide resolved
src/kerberos/client/generators.rs Outdated Show resolved Hide resolved
@CBenoit CBenoit changed the title Fix: the GSS Flags were hard coded, which does not work for Ldap fix: properly map SSPI flags to Kerberos GSSAPI flags Dec 13, 2023
Co-authored-by: Benoît Cortier <bcortier@proton.me>
@irvingoujAtDevolution
Copy link
Contributor Author

@awakecoding , one thing to notice is that, this flag should also has an effect when encrypt/decrypt payloads. For example, if sign is on and seal is off, the message should comes with a signature but with unencrypted payload. I'll open a issue to track it

@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Dec 13, 2023

For future reference.

image

here is a screenshot that it works no sign, no seal, plain TCP connection

image
here is another that works with no sign, no seal, tls connection but decrypted using @awakecoding TLS decryption script

Co-authored-by: Benoît Cortier <bcortier@proton.me>
Copy link
Collaborator

@TheBestTvarynka TheBestTvarynka left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@irvingoujAtDevolution irvingoujAtDevolution merged commit 550463e into master Dec 13, 2023
40 checks passed
@irvingoujAtDevolution irvingoujAtDevolution deleted the Fix-Hard-Coded-GSS-FLAG branch December 13, 2023 18:37
@jborean93
Copy link

jborean93 commented Dec 20, 2023

Note: This change might be breaking. Confidentiality and integrity is not implied anymore. User needs to actively specify these two flag to get sign and seal.

Don't you always want confidentiality and integrity by default, you should only not be setting it if the NO_INTEGRITY flag is specified.

It looks good to me, I'm surprised it has worked so far. We'll need to test that it doesn't break things for other protocols though

AFAIK LDAP (when using LDAPS or LDAP + StartTLS) is the only one that has needed this and specifically with the GSS-SPNEGO SASL. I've not encountered any other protocols so far that requires explicitly disabling integrity/confidentiality with the auth.

@pauldumais
Copy link
Contributor

Note: This change might be breaking. Confidentiality and integrity is not implied anymore. User needs to actively specify these two flag to get sign and seal.

Don't you always want confidentiality and integrity by default, you should only not be setting it if the NO_INTEGRITY flag is specified.

LDAPS does not allow sealing since the connection is already encrypted.

@jborean93
Copy link

Only when using GSS-SPNEGO because it uses the negotiated context flags to determine if signing/sealing was applied by the auth context. The GSSAPI SASL is fine with this as it negotiates the signing/sealed after the auth with another round trip of tokens. My comment was around not always having integrity and confidentiality by default and opting out of it with the NO_INTEGRITY flag. I.e. both SSPI and GSSAPI always negotiate integrity and confidentiality with Kerberos unless the relevant NO_INTEGIRTY flag is specified.

@pauldumais
Copy link
Contributor

Yes agreed we will make sure to fix this.

@irvingoujAtDevolution
Copy link
Contributor Author

@jborean93 added new issue #198 according to the thread.

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

Successfully merging this pull request may close these issues.

7 participants