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

Allow stdin input of cert for needs-renewal #1157

Merged
merged 1 commit into from
May 7, 2024

Conversation

redrac
Copy link
Contributor

@redrac redrac commented Apr 20, 2024

Name of feature:

Allow stdin input of cert for needs-renewal

Pain or issue this feature alleviates:

This allows users who have ephemeral certs (certs only in agents) to use needs-renewal without writing the cert to disk. This functionality already exists for inspect; I merely ported it over.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Apr 20, 2024
@redrac redrac force-pushed the feature/allow_stdin_keys branch from 7065c1a to ba1685d Compare April 23, 2024 14:21
@hslatman hslatman requested a review from dopey April 23, 2024 17:16
@redrac
Copy link
Contributor Author

redrac commented Apr 24, 2024

Now requires: smallstep/crypto#490
Related to: #116

edit: these were merged

@redrac redrac force-pushed the feature/allow_stdin_keys branch from ba1685d to 2e78bf9 Compare April 26, 2024 21:57
@dopey
Copy link
Contributor

dopey commented May 3, 2024

Hey @redrac 👋. Thanks for submitting this PR, and apologies for the radio silence!

Overall, this looks good and useful. However, the exit codes that are being returned by this command are intentional and in some cases I think the proposed changes may result in an exit code that does not line up with the documentation. I will try to comment on those exact cases.

See the exit code documentation here - https://github.com/smallstep/cli/blob/master/command/ssh/needsRenewal.go#L39-L43.

case 1:
name = ctx.Args().First()
default:
return errs.TooManyArguments(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we want to exit with code 255 here.

@redrac
Copy link
Contributor Author

redrac commented May 3, 2024

Hey @redrac 👋. Thanks for submitting this PR, and apologies for the radio silence!

Overall, this looks good and useful. However, the exit codes that are being returned by this command are intentional and in some cases I think the proposed changes may result in an exit code that does not line up with the documentation. I will try to comment on those exact cases.

See the exit code documentation here - https://github.com/smallstep/cli/blob/master/command/ssh/needsRenewal.go#L39-L43.

Ah thanks! I will fix this this afternoon most likely

@redrac redrac force-pushed the feature/allow_stdin_keys branch from 2e78bf9 to f36bb35 Compare May 3, 2024 21:06
@redrac redrac force-pushed the feature/allow_stdin_keys branch from f36bb35 to 17f02aa Compare May 3, 2024 21:10
@redrac
Copy link
Contributor Author

redrac commented May 3, 2024

Okay fixed :)

$ ./step ssh needs-renewal
too many arguments: not enough positional arguments were provided in 'step ssh needs-renewal <crt-file>'
$ echo $?
255

$ ./step ssh needs-renewal zz zzz zz
too many arguments: too many positional arguments were provided in 'step ssh needs-renewal <crt-file>'
$ echo $?
255

@redrac redrac requested a review from dopey May 3, 2024 21:10
@dopey dopey merged commit 1f36d23 into smallstep:master May 7, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants