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

Remove the need for a keytab when using fast with anonymous pkinit #6624

Closed
wants to merge 1 commit into from

Conversation

lukeadickinson
Copy link

I created an issue describing the reason for the change: #6531

In short, SSSD supports fast with anonymous pkinit. Anonymous pkinit does not need a keytab file to function. However, the current sssd source always attempts to copy a keytab into memory when fast is being used even if anonymous pkinit is being used. If the keytab file is missing, sssd will crash and authentication will not work.

DEBUG(SSSDBG_OP_FAILURE, "copy_keytab_into_memory failed.\n");
return kerr;
/* A Keytab is not used if fast with anonymous pkinit is used (and validate is false)*/
if(kr->validate || kr->cli_opts->fast_use_anonymous_pkinit == false){
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

thank you for the fix. Would you mind to use kr->validate == true to be more consistent with the other comparisons?

Additionally, please add Resolves: https://github.com/SSSD/sssd/issues/6531 at the end of the commit message.

bye,
Sumit

Copy link
Author

Choose a reason for hiding this comment

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

I took a little different approach to make my change more consistent than you recommended. I believe my new change is clearer. If you would prefer me to use your recommendation let me know and I can make the change.

I think I did the resolve comment correctly. Please let me know if I missed something.

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thank you for the update, ACK.

bye,
Sumit

Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@pbrezina
Copy link
Member

Hi, make check fails on whitespace test.

Trailing whitespace found:
../src/providers/krb5/krb5_child.c:3686:        if(!(kr->cli_opts->fast_use_anonymous_pkinit == true && kr->validate == false)){ 
../src/providers/krb5/krb5_child.c:3693:            
../src/providers/krb5/krb5_child.c:3697:        
FAIL src/tests/whitespace_test (exit status: 1)

Can you please fix that? Thank you.

@lukeadickinson
Copy link
Author

I believe I made all of the requested changes. Did you need any other changes from me?

@sumit-bose
Copy link
Contributor

I believe I made all of the requested changes. Did you need any other changes from me?

Hi,

it looks you only fixed the one in line 3686, there are still issues with line 3693 and 3697 (only white-spaces, otherwise the line is empty).

bye,
Sumit

@lukeadickinson
Copy link
Author

Oh. That is a bit embarrassing, I didn't realize how to run the tests myself (I do now), and I didn't notice the lines that were just spaces from pbrezina's message above.

FYI: When I run make check on my local machine, "file_watch-tests" fail. I don't know what that is, and I assume my changes didn't cause that.

I also see that covscan is failing, but again I am not sure why and I assume my changes aren't related.

In the future, Is there a way I can see the "test-suite.log" on github? I found it when I ran the tests myself. Its not important either way, just curious.

@sumit-bose
Copy link
Contributor

Hi,

thanks for the update. The covscan issue is indeed not related to your patch and it even looks like a false positive to me. Not sure why file_watch-tests are failing for you, maybe something related to inotify is not working as expected in your environment?

bye,
Sumit

@pbrezina pbrezina added Ready to push Ready to push and removed Accepted Ready to push Ready to push labels Apr 17, 2023
@pbrezina
Copy link
Member

Hi @lukeadickinson thank you for your contribution. Before we merge it, could you please remove the merge commit and then squash it all into a single commit with a nice subject and description?

Anonymous pkinit does rely on a keytab for authentication.
Removed unnecessary call to copy keytab into memory.

Resolves: SSSD#6531
@lukeadickinson
Copy link
Author

I undid my previous commits and created a single commit with my changes.

Let me know if there is anything else I need to do.

Thanks!

return kerr;
}
/* A Keytab is not used if fast with anonymous pkinit is used (and validate is false)*/
if(!(kr->cli_opts->fast_use_anonymous_pkinit == true && kr->validate == false)){
Copy link
Member

Choose a reason for hiding this comment

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

Please, add space after if and before { to make it compatible with our coding style. But I can do it prior merging it if you want me to.

Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

If you would be willing to add the spaces and adjust the spacing for the NULL in your comment below, please go ahead. If you would prefer I do it, I can.

/* A Keytab is not used if fast with anonymous pkinit is used (and validate is false)*/
if(!(kr->cli_opts->fast_use_anonymous_pkinit == true && kr->validate == false)){
kerr = copy_keytab_into_memory(kr, kr->ctx, kr->keytab, &mem_keytab,
NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Please, align NULL with kr.

@pbrezina
Copy link
Member

pbrezina commented May 4, 2023

  • master
    • d486694 - Remove the need for a keytab when using fast with anonymous pkinit

@pbrezina pbrezina closed this May 4, 2023
@sumit-bose
Copy link
Contributor

Hi,

the squashed version is the same and works in my tests, ACK.

bye,
Sumit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted no-backport This should go to target branch only. Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants