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

add nullok support to challenge-response mode #121

Merged
merged 4 commits into from Jun 26, 2017
Merged

Conversation

Larhard
Copy link
Contributor

@Larhard Larhard commented Apr 9, 2017

Hi, I wanted to use nullok flag with challenge-response mode so I added the support. I hope it will be helpful.

It solves issue "Cannot login with nullok using challenge-response? #119"

Copy link
Member

@klali klali left a comment

Choose a reason for hiding this comment

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

Thanks for your interest in this!

I've added a couple of small notes on changes I'd like to see for this. Additionally I'd like there to be some tests for the new function check_user_challenge_file() in util_tests.c.

util.c Outdated
*
* Returns one of AUTH_FOUND, AUTH_NOT_FOUND, AUTH_ERROR
*/
int len;
Copy link
Member

Choose a reason for hiding this comment

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

len should probably be a size_t since it's assigned result of strlen(), passed to malloc() and passed to snprintf()

@@ -282,6 +283,64 @@ int challenge_response(YK_KEY *yk, int slot,
}

int
check_user_challenge_file(const char *chalresp_path, const struct passwd *user, FILE *debug_file)
Copy link
Member

Choose a reason for hiding this comment

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

debug_file isn't used in the function, consider dropping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some error messages instead


r = glob(userfile_pattern, 0, NULL, &userfile_glob);
globfree(&userfile_glob);
switch (r) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer for all cases in the switch to look the same, setting ret+break, this decreases risks of messing this up in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment shoud explain my idea. I would keep it that way (moved break to the top and then ret+goto out)

@Larhard
Copy link
Contributor Author

Larhard commented Apr 27, 2017

In a free moment I will fix this, but for now I didn't have any.

@klali
Copy link
Member

klali commented Jun 26, 2017

Ok. Merging this, thank you!

@klali klali merged commit 42e8a06 into Yubico:master Jun 26, 2017
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.

None yet

2 participants