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

GUACAMOLE-1068: Generate random key at each login if key is unconfirmed. #974

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

necouchman
Copy link
Contributor

A simple take at randomizing the key for users who have not confirmed enrollment. I'm not sure if this introduces any other gotchas or security issues - it almost seems too simple?

@mike-jumper
Copy link
Contributor

Hm ... I'd think this would result in the TOTP key getting regenerated when the user attempts to enroll:

  1. User attempts to log in.
  2. Key is generated and presented to user for enrollment.
  3. To confirm enrollment, the user resubmits credentials plus the TOTP code for that key.
  4. That's a login attempt and the user isn't confirmed yet, so the key is regenerated.
  5. The TOTP code doesn't match the newly-generated key and the user is blocked from enrolling.

@necouchman
Copy link
Contributor Author

Hm ... I'd think this would result in the TOTP key getting regenerated when the user attempts to enroll:

  1. User attempts to log in.
  2. Key is generated and presented to user for enrollment.
  3. To confirm enrollment, the user resubmits credentials plus the TOTP code for that key.
  4. That's a login attempt and the user isn't confirmed yet, so the key is regenerated.
  5. The TOTP code doesn't match the newly-generated key and the user is blocked from enrolling.

Ah, okay - I thought it was probably too simple...and haven't thoroughly tested it, yet. Will convert to draft and take another look.

@necouchman necouchman marked this pull request as draft April 12, 2024 22:31
@necouchman necouchman marked this pull request as ready for review April 13, 2024 03:11
@necouchman
Copy link
Contributor Author

Okay, this change seems to work as expected, now.

Copy link
Contributor

@jmuehlner jmuehlner left a comment

Choose a reason for hiding this comment

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

LGTM. I also tested it real quick and it does indeed seem to work as expected.

@jmuehlner jmuehlner merged commit e9bac2a into apache:main Apr 18, 2024
1 check passed
@necouchman
Copy link
Contributor Author

LGTM. I also tested it real quick and it does indeed seem to work as expected.

Thanks!

@necouchman necouchman deleted the jira/1068 branch April 26, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants