Rework krb5cc handling logic #699

GabrielNagy · 2023-05-16T08:52:53Z

Following the Heimdal issues surfaced after the samba 4.16.0 release we have to rethink the krb5cc handling logic with the following in mind:

we can no longer use symlinks as Kerberos tickets
the ticket has to be owned by the running user (in our case root)

This is not related to a change in samba itself, but to the Kerberos implementation vendored in samba (Heimdal Kerberos). Between samba 4.15.0 and 4.16.0 there has been a massive update in the vendored code, pulling in around 10 years worth of changes. Among these changes we were able to find information confirming the 2 issues above, and we concluded they are not considered to be bugs in the upstream implementation (and related issue).

Another important thing to note is that the issues above do not manifest if samba is compiled and linked against the MIT Kerberos library. However, MIT Kerberos appears to still be considered experimental (as of today), and neither Ubuntu or other distros are compiling samba with MIT Kerberos.

As such, we have to work around these "issues" within adsys. Instead of tracking the ticket's lifetime using a symlink, we rely on the following approach:

still track original ticket with a symlink
copy the ticket as a root-owned ticket
track refresh with timestamps on the symlink itself.

Symlinks will remain stored under KRB5CACHEDIR, whereas the actual copies will reside under KRB5CACHEDIR/tickets.

Closes UDENG-289

denisonbarbosa

Looks very nice, well done! I have some suggestions that I feel like should be attended, but otherwise: amazing work!

internal/ad/ad.go

codecov-commenter · 2023-05-16T18:22:16Z

Codecov Report

Merging #699 (326decb) into main (18c04e1) will decrease coverage by 0.16%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##             main     #699      +/-   ##
==========================================
- Coverage   84.73%   84.57%   -0.16%     
==========================================
  Files          75       75              
  Lines        8177     8229      +52     
==========================================
+ Hits         6929     6960      +31     
- Misses        936      950      +14     
- Partials      312      319       +7

Impacted Files	Coverage Δ
internal/ad/ad.go	`86.82% <62.50%> (-3.80%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

denisonbarbosa

Looks great! Well done for spotting and fixing it!

didrocks

Nice work! It’s always almost a deception when you see how much the investigation took compared to the "fix" (or rather workaround here). Well done!

I have one small request to make the whole logic more understandable in the future (I think, as an admin, I open the cache and see directly what is in use).

However, I wonder about the coverage. Codecov is not really helpful as it doesn’t show up the file diff change. Do you think that some tests in ad_test.go could be added to trigger some uncovered tests and not only relying on the integration tests to cover it?

(I’m thinking in particular about a test with ticket, ad, get it, then updating the ticket, ad away, -> copy was refreshed)

internal/ad/ad.go

cmd/adsysd/integration_tests/adsysctl_policy_test.go

GabrielNagy · 2023-05-23T16:00:38Z

@didrocks sorry for the aggresive force-pushing 😊 the only changes are in the first and last commit (which is newly added).

didrocks · 2023-05-23T16:03:14Z

@didrocks sorry for the aggresive force-pushing blush the only changes are in the first and last commit (which is newly added).

I was on the starting block to complain, but now that you apologized… :p

didrocks

I’m not sure I’m fully following the logic here (in addition to the comment I mentioned).

I don’t see the symlink to be reused anywhere after being created. The idea is that any "all policy updates" (so, without explicit env variable to get the ticket), should get the real ticket location from the symlink and assess if a copy refresh is in order? I may be missing it completely, but I even reopened the file to see if there is any hidden logic I’m missing.

If I am correct, it means that we are missing a test to setup something for a given user, then, modifying ticket and ask for a refresh without pointing to that user in particular, and check that the copy has been refreshed.

cmd/adsysd/integration_tests/adsysctl_service_test.go

internal/ad/ad_test.go

internal/ad/ad.go

GabrielNagy · 2023-05-23T18:07:56Z

I don’t see the symlink to be reused anywhere after being created. The idea is that any "all policy updates" (so, without explicit env variable to get the ticket), should get the real ticket location from the symlink and assess if a copy refresh is in order? I may be missing it completely, but I even reopened the file to see if there is any hidden logic I’m missing.

That's a very good observation which I totally missed, thanks for that. I've now updated the implementation to attempt the copy even if an explicit krb5ccname is not passed and validated the behavior with a package test.

didrocks · 2023-05-24T06:34:05Z

That's a very good observation which I totally missed, thanks for that. I've now updated the implementation to attempt the copy even if an explicit krb5ccname is not passed and validated the behavior with a package test.

No worry, I think that shows we should have an automated test for this behavior in the ad package, wdyt?

GabrielNagy · 2023-05-24T07:13:08Z

I agree, that's why I added one :P. Let me know if this is what you had in mind.

didrocks

Excellent, one last comment on the test name, then feel free to merge :)

Woow, good to see this merged, especially with, in the end, some improvements with the symlink checked :) Well done!

cmd/adsysd/integration_tests/adsysctl_policy_test.go

Following the Heimdal issues surfaced after the samba 4.16.0 release we have to rethink the krb5cc handling logic with the following in mind: - we can no longer use symlinks as Kerberos tickets - the ticket has to be owned by the running user (in our case root) This is not related to a change in samba itself, but to the Kerberos implementation vendored in samba (Heimdal Kerberos). Between samba 4.15.0 and 4.16.0 there has been a massive update in the vendored code, pulling in around 10 years worth of changes[1]. Among these changes we were able to find information confirming the 2 issues above, and we concluded they are not considered to be bugs in the upstream implementation[2] (and related issue [3]). Another important thing to note is that the issues above do not manifest if samba is compiled and linked against the MIT Kerberos library. However, MIT Kerberos appears to still be considered experimental (as of today), and neither Ubuntu or other distros are compiling samba with MIT Kerberos. As such, we have to work around these "issues" within adsys. Instead of tracking the ticket's lifetime using a symlink, we rely on the following approach: * still track original ticket with a symlink * copy the ticket as a root-owned ticket * track refresh with timestamps on the symlink itself. Ticket copies will be stored under KRB5CACHEDIR, whereas symlinks will reside under KRB5CACHEDIR/tracking. Closes UDENG-289 [1] samba-team/samba@40b65c8 [2] heimdal/heimdal@2a56548 [3] heimdal/heimdal#306

I believe that with the previous commit we've uncovered a small bug in the krb5cc symlink logic. More specifically, in these tests we were creating symlinks to a nonexistent path (ccache_OFFLINE) due to the sssd configuration. Because the presence of the source file wasn't enforced in `ensureKrb5CCName` (filepath.Abs doesn't ensure the path actually exists), this went unnoticed until now. This was only a problem in the offline tests which didn't go to our samdb mock, which would have caught the dangling symlink issue. Now that we do the additonal copy action the failure becomes visible. The fix is simple, we just have to create the cached sssd ticket using the appropriate domain name.

Employ a write to temp & rename workflow to ensure we're not left in a broken state if the write fails midway.

When checking for active users, symlinks are now followed, and users with dangling symlinks will now be silently ignored.

When upgrading from a previous version of the package it's highly likely to have the previous krb5cc structure in /var/run/krb5cc, with symlinks at the top level. We need to make sure we overwrite previous symlinks with the hard copy in this case.

GabrielNagy force-pushed the krb5cc-symlink-handling branch 3 times, most recently from 7da1eeb to 5cb1f6c Compare May 16, 2023 12:21

GabrielNagy marked this pull request as ready for review May 16, 2023 12:47

GabrielNagy requested a review from a team as a code owner May 16, 2023 12:47

denisonbarbosa requested changes May 16, 2023

View reviewed changes

internal/ad/ad.go Outdated Show resolved Hide resolved

internal/ad/ad.go Outdated Show resolved Hide resolved

internal/ad/ad.go Outdated Show resolved Hide resolved

internal/ad/ad.go Outdated Show resolved Hide resolved

GabrielNagy force-pushed the krb5cc-symlink-handling branch from 5cb1f6c to 326decb Compare May 16, 2023 17:55

GabrielNagy force-pushed the krb5cc-symlink-handling branch from 326decb to b269169 Compare May 17, 2023 17:43

denisonbarbosa approved these changes May 18, 2023

View reviewed changes

didrocks requested changes May 23, 2023

View reviewed changes

internal/ad/ad.go Outdated Show resolved Hide resolved

internal/ad/ad.go Show resolved Hide resolved

cmd/adsysd/integration_tests/adsysctl_policy_test.go Show resolved Hide resolved

GabrielNagy force-pushed the krb5cc-symlink-handling branch 2 times, most recently from 54583e2 to fd98e17 Compare May 23, 2023 15:46

didrocks requested changes May 23, 2023

View reviewed changes

cmd/adsysd/integration_tests/adsysctl_service_test.go Show resolved Hide resolved

internal/ad/ad_test.go Show resolved Hide resolved

internal/ad/ad.go Show resolved Hide resolved

didrocks approved these changes May 24, 2023

View reviewed changes

cmd/adsysd/integration_tests/adsysctl_policy_test.go Outdated Show resolved Hide resolved

GabrielNagy added 3 commits May 24, 2023 12:01

Copy kerberos ticket atomically

30fe178

Employ a write to temp & rename workflow to ensure we're not left in a broken state if the write fails midway.

GabrielNagy force-pushed the krb5cc-symlink-handling branch from 2390a97 to 2097c9c Compare May 24, 2023 10:46

GabrielNagy added 2 commits May 24, 2023 14:44

Rework listing active users

b5a4db3

When checking for active users, symlinks are now followed, and users with dangling symlinks will now be silently ignored.

GabrielNagy force-pushed the krb5cc-symlink-handling branch from f7484bd to 535d60f Compare May 24, 2023 11:44

GabrielNagy merged commit ba00458 into main May 24, 2023
5 checks passed

GabrielNagy deleted the krb5cc-symlink-handling branch May 24, 2023 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework krb5cc handling logic #699

Rework krb5cc handling logic #699

GabrielNagy commented May 16, 2023

denisonbarbosa left a comment

codecov-commenter commented May 16, 2023

denisonbarbosa left a comment

didrocks left a comment

GabrielNagy commented May 23, 2023

didrocks commented May 23, 2023

didrocks left a comment

GabrielNagy commented May 23, 2023

didrocks commented May 24, 2023

GabrielNagy commented May 24, 2023

didrocks left a comment

Rework krb5cc handling logic #699

Rework krb5cc handling logic #699

Conversation

GabrielNagy commented May 16, 2023

denisonbarbosa left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 16, 2023

Codecov Report

denisonbarbosa left a comment

Choose a reason for hiding this comment

didrocks left a comment

Choose a reason for hiding this comment

GabrielNagy commented May 23, 2023

didrocks commented May 23, 2023

didrocks left a comment

Choose a reason for hiding this comment

GabrielNagy commented May 23, 2023

didrocks commented May 24, 2023

GabrielNagy commented May 24, 2023

didrocks left a comment

Choose a reason for hiding this comment