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.

Sign up for GitHub

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

Jump to bottom

Fix certificate extension not being included in tctl auth sign #10949

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Fix certificate extension not being included in tctl auth sign #10949

merged 1 commit into from
Mar 15, 2022

Conversation

lxea
Copy link
Contributor

@lxea lxea commented Mar 8, 2022

This meant that certificates exported with tctl auth sign were not including the ssh key extensions.

@github-actions github-actions bot requested review from gabrielcorado and r0mant March 8, 2022 12:19
@zmb3 zmb3 requested a review from probakowski March 8, 2022 14:29
probakowski
probakowski approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@probakowski probakowski left a comment

Choose a reason for hiding this comment

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

LGTM 👍 would be good to add test to make sure it really works

@lxea lxea force-pushed the lxea/fix-cert-extensions-auth-sign branch 2 times, most recently from b1e32ed to 59bfec5 Compare March 8, 2022 15:14
@lxea lxea enabled auto-merge (rebase) March 8, 2022 15:47
@lxea lxea force-pushed the lxea/fix-cert-extensions-auth-sign branch from 59bfec5 to 84a14ee Compare March 9, 2022 10:20
probakowski
probakowski requested changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@probakowski probakowski 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 adding test, I think last check can be simplified

lib/auth/auth_test.go Outdated Show resolved Hide resolved
@lxea lxea force-pushed the lxea/fix-cert-extensions-auth-sign branch from 84a14ee to 26cc286 Compare March 9, 2022 14:42
probakowski
probakowski approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@probakowski probakowski 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 @lxea !

@lxea lxea force-pushed the lxea/fix-cert-extensions-auth-sign branch from 26cc286 to d70d1ba Compare March 14, 2022 11:42
r0mant
r0mant approved these changes Mar 15, 2022
View reviewed changes
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

@lxea Let's get this in and backport to v9 also (if the cert. extensions are present in v9).

@lxea lxea force-pushed the lxea/fix-cert-extensions-auth-sign branch 2 times, most recently from b37d0ee to 1efe32b Compare March 15, 2022 13:03
@lxea lxea force-pushed the lxea/fix-cert-extensions-auth-sign branch from 1efe32b to ee3d365 Compare March 15, 2022 13:23
@lxea lxea merged commit 26f7f17 into master Mar 15, 2022
@lxea lxea deleted the lxea/fix-cert-extensions-auth-sign branch March 15, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r0mant r0mant r0mant approved these changes

@gabrielcorado gabrielcorado gabrielcorado approved these changes

@probakowski probakowski probakowski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lxea @r0mant @gabrielcorado @probakowski