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

Fixes for kerberos authentication: #6661

Closed
wants to merge 1 commit into from

Conversation

akallabeth
Copy link
Member

@akallabeth akallabeth commented Dec 11, 2020

  • Negotiate: Do a proper check if a realm is set up and ready,
    otherwise fall back to NTLM
  • NTLM: Return error if FIPS is activated
  • Negotiate: Ignore NTLM if FIPS is activated
  • Removed GSS related error checks in freerdp, moved entirely
    to WinPR

TODO:

  • Set proper error states in case of authentication failures (can be done later on, better than what is currently on master)

@akallabeth akallabeth marked this pull request as draft December 11, 2020 07:37
@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5456/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5457/

@ondrejholy
Copy link
Contributor

Thanks for working on this!

Request a new ticket if none is found (thsi code had to be removed due to overwriting existing tickets and other stuff)

I am pretty sure it is not desired to request TGT if none was found. So it is right to fallback to NTLM. This is how Kerberos applications work. It is up to the user to obtain the ticket itself or use some daemon for it. If you have some doubts about it, @abbra could possibly give you a more sophisticated explanation...

@abbra
Copy link

abbra commented Dec 11, 2020

Please do not fallback to NTLM by default or at least make it configurable. In FIPS mode you cannot use RC4 cipher so it will fail anyway.

@ondrejholy
Copy link
Contributor

My point was to just draw the attention that it is not desired to call krb5_init from the FreeRDP as it is done in master currently.

You are right that Kerberos and NTLM are probably handled together as NLA, so it would be nice to add some specific options for this.

FreeRDP should currently automatically disable NLA security on FIPS. So maybe this logic needs to be updated to disable only NTLM, not Kerberos.

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5461/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5475/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5476/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5491/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5492/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5493/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5547/

* Negotiate: Do a proper check if a realm is set up and ready,
  otherwise fall back to NTLM
* Removed GSS related error checks in freerdp, moved entirely
  to WinPR
@akallabeth
Copy link
Member Author

@freerdp-bot test

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5706/

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5707/

@akallabeth akallabeth added this to the next milestone Mar 1, 2021
@akallabeth akallabeth marked this pull request as ready for review March 1, 2021 12:23
@akallabeth
Copy link
Member Author

@bmiklautz can you install libgss-dev on the ubuntu build machines please?

@akallabeth
Copy link
Member Author

@freerdp-bot test

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5744/

@SergejFrank
Copy link

@freerdp-bot test

1 similar comment
@bmiklautz
Copy link
Member

@freerdp-bot test

@freerdp-bot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/5982/

#ifdef UNICODE
ConvertToUnicode(CP_UTF8, 0, principal, -1, &nla->ServicePrincipalName, 0);
#else
nla->ServicePrincipalName = _strdup(principal);
Copy link
Contributor

Choose a reason for hiding this comment

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

missing checks

Copy link
Contributor

@hardening hardening left a comment

Choose a reason for hiding this comment

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

Changes look good for me (would need a rebase though).
I'm very uncomfortable with the Negotiate package which is badly named. We would expect it to do the full (and complicated) SPnego stuff as its windows counterpart, and instead of that it's just an implementation switcher between NTLM and Kerberos.

@lhoward
Copy link

lhoward commented Sep 3, 2021

Changes look good for me (would need a rebase though).
I'm very uncomfortable with the Negotiate package which is badly named. We would expect it to do the full (and complicated) SPnego stuff as its windows counterpart, and instead of that it's just an implementation switcher between NTLM and Kerberos.

The SPNEGO implementations in MIT and Heimdal have had many years of testing (and indeed both now support NegoEx), so perhaps using them would be a good idea? You could handle NTLM either explicitly, which should work for most cases, or by using a NTLM GSS mechanism (of which there are a few, e.g. see here).

@abbra
Copy link

abbra commented Sep 3, 2021

I agree with Luke. We have quite good SPNEGO implementations in MIT Kerberos 5 and Heimdal and with gss-ntlmssp we can cover the remaining part of NTLMSSP support. What's more important, both MIT and Heimdal versions also do support MS-NEGOEX infrastructure, making it possible to use other authentication mechanisms supported by Windows and, in future, by Linux environments.

@hardening
Copy link
Contributor

Side note: yes MIT Kerberos 5 and heimdal have everything to do SPNego client-side, but as they don't implement the user2user microsoft kerberos extension, on the server-side it's impossible to accept mstsc with NLA and kerberos (except if you develop you own krb5 extension library for this OID).

I'd like to describe what's the behaviour of mstsc, just to measure that SPNego is not that magical:

  • mstsc tries to figure out if kerberos would work with the target server, checking its credentials cache (can't remember if it tries to get a service ticket), etc;
  • if it could work then it connects using SPNego, advertising a user2user optimistic token;
  • if kerberos would not work, then it connects directly with a NTLM token, no SPNego at all;

note: rdesktop goes directly with a kerberos token, and that works fine against windows servers

So in theory SPNego allows to negotiate things, but in practise this capability is not used, SPNego is used just because it allows the exchange of MICs in the user2user workflow.

So we have a subject with kerberos, I'm really wondering how we can deal with the 2 use cases where:

  • the user have not initialized anything with kerberos and so FreeRDP will create a credentials cache to access that domain (using the provided credentials ?);
  • the user (or FreeRDP previously) has already logged on the domain, and so connecting is just a matter of getting a TGS;

@lhoward
Copy link

lhoward commented Sep 3, 2021

Just briefly as I'm at dinner: modern Kerberos implementations implement gss_acquire_cred_from() which let you specify a set of key values containing password, keytab name, credentials cache, etc – so it is possible to easily handle default and non-default credentials.

Re: using off-the-shelf SPNEGO, would it help if we implemented the MS user2user Kerberos mechanism? I haven't read the RDP protocol spec enough to know exactly why user2user is used.

@lhoward
Copy link

lhoward commented Sep 3, 2021

Not saying you need to replace your SPNEGO implementation of course, but there are a lot of subtleties in the protocol, particularly when it comes to generating/verifying mechListMIC, not to mention supporting other mechanisms inside NegoEx. I was still fixing bugs in Heimdal's mechListMIC implementation as recently as last year.

@lhoward
Copy link

lhoward commented Sep 3, 2021

Also, any intention to support Remote Credential Guard?

@akallabeth
Copy link
Member Author

@lhoward interesting. we´d need to check that.
Currently this pr is on hold as I do not have time to spend on this, but help is very welcome.

As for remote credential guard, I´ve also did not have time for this yet.

@hardening
Copy link
Contributor

@lhoward I gave a look at remote credential guards some time ago, and there's really a lot to implement as MS-RDPEAR has quite a lot of messages.

@hardening
Copy link
Contributor

@akallabeth is there anything that is not fixed in #7934 in this PR ?

@akallabeth
Copy link
Member Author

guess not, closing.

@akallabeth akallabeth closed this Jun 21, 2022
@akallabeth akallabeth removed this from the next milestone Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add kerberos single sign-on support
8 participants