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

Update init.go adding dnsValue Validation at interactive prompt #812

Closed
wants to merge 2 commits into from

Conversation

jacostaperu
Copy link

Name of feature:

dnsValue validation to prevent panic when it is empty

Pain or issue this feature alleviates:

currently when calling step ca init it accept empty string for dnsValue causing panic later

Ô×£  certificates git:(master) STEPDEBUG=1 step ca init
Ô£ö Deployment Type: Standalone
What would you like to name your new PKI?
Ô£ö (e.g. Smallstep): SmallStep
What DNS names or IP addresses would you like to add to your new CA?     
Ô£ö (e.g. ca.smallstep.com[,1.1.1.1,etc.]):    <- leave it empty
What IP and port will your new CA bind to?
Ô£ö (e.g. :443 or 127.0.0.1:443): :8443
What would you like to name the CA's first provisioner?
Ô£ö (e.g. [email protected]): [email protected]
Choose a password for your CA keys and first provisioner.
Smallstep CLI/0.21.0 (linux/amd64)
Release Date: 2022-07-06T22:23:54Z

panic: runtime error: index out of range [0] with length 0 [recovered]
        panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
main.panicHandler()
        ./main.go:139 +0x245
panic({0x12dec00, 0xc00003e1c8})
        runtime/panic.go:838 +0x207
github.com/smallstep/certificates/pki.New({{0x137b07c, 0x7}, {0x0, 0x0}, {0x0, 0x0}, 0x0, {0x0, 0x0}, {0x0, ...}, ...}, ...)
        github.com/smallstep/[email protected]/pki/pki.go:382 +0xfe5
github.com/smallstep/cli/command/ca.initAction(0xc00055c2c0)
        github.com/smallstep/cli/command/ca/init.go:563 +0x3419
github.com/urfave/cli.HandleAction({0x11bec80?, 0x1467900?}, 0x4?)
        github.com/urfave/[email protected]/app.go:522 +0x7f
github.com/urfave/cli.Command.Run({{0x1372454, 0x4}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x13a51fc, 0x15}, {0x14122b5, ...}, ...}, ...)
        github.com/urfave/[email protected]/command.go:173 +0x652
github.com/urfave/cli.(*App).RunAsSubcommand(0xc0006ed880, 0xc00055c000)
        github.com/urfave/[email protected]/app.go:405 +0x91b
github.com/urfave/cli.Command.startApp({{0x137001e, 0x2}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x13d6304, 0x2d}, {0x13eff37, ...}, ...}, ...)
        github.com/urfave/[email protected]/command.go:372 +0x6e7
github.com/urfave/cli.Command.Run({{0x137001e, 0x2}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x13d6304, 0x2d}, {0x13eff37, ...}, ...}, ...)
        github.com/urfave/[email protected]/command.go:102 +0x808
github.com/urfave/cli.(*App).Run(0xc0006ed6c0, {0xc00003a180, 0x3, 0x3})
        github.com/urfave/[email protected]/app.go:277 +0x8a7
main.main()
        ./main.go:113 +0x611

Why is this important to the project (if not answered above):

I believe it will prevent code panics on that particular scenario.

Is there documentation on how to use this feature? If so, where?

In what environments or workflows is this feature supported?

In what environments or workflows is this feature explicitly NOT supported (if any)?

Supporting links/other PRs/issues:

💔Thank you!

Adding validation of dns value at prompt time to prevent panic error later.
currently it accepts empty values and cause program panic 
e.g. 

 STEPDEBUG=1 step ca init
...
 (e.g. ca.smallstep.com[,1.1.1.1,etc.]):
....

Smallstep CLI/0.21.0 (linux/amd64)
Release Date: 2022-07-06T22:23:54Z

panic: runtime error: index out of range [0] with length 0 [recovered]
        panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
main.panicHandler()
        ./main.go:139 +0x245
panic({0x12dec00, 0xc00003e1c8})
        runtime/panic.go:838 +0x207
github.com/smallstep/certificates/pki.New({{0x137b07c, 0x7}, {0x0, 0x0}, {0x0, 0x0}, 0x0, {0x0, 0x0}, {0x0, ...}, ...}, ...)
        github.com/smallstep/[email protected]/pki/pki.go:382 +0xfe5
github.com/smallstep/cli/command/ca.initAction(0xc00055c2c0)
        github.com/smallstep/cli/command/ca/init.go:563 +0x3419
github.com/urfave/cli.HandleAction({0x11bec80?, 0x1467900?}, 0x4?)
        github.com/urfave/[email protected]/app.go:522 +0x7f
github.com/urfave/cli.Command.Run({{0x1372454, 0x4}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x13a51fc, 0x15}, {0x14122b5, ...}, ...}, ...)
        github.com/urfave/[email protected]/command.go:173 +0x652
github.com/urfave/cli.(*App).RunAsSubcommand(0xc0006ed880, 0xc00055c000)
        github.com/urfave/[email protected]/app.go:405 +0x91b
github.com/urfave/cli.Command.startApp({{0x137001e, 0x2}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x13d6304, 0x2d}, {0x13eff37, ...}, ...}, ...)
        github.com/urfave/[email protected]/command.go:372 +0x6e7
github.com/urfave/cli.Command.Run({{0x137001e, 0x2}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x13d6304, 0x2d}, {0x13eff37, ...}, ...}, ...)
        github.com/urfave/[email protected]/command.go:102 +0x808
github.com/urfave/cli.(*App).Run(0xc0006ed6c0, {0xc00003a180, 0x3, 0x3})
        github.com/urfave/[email protected]/app.go:277 +0x8a7
main.main()
        ./main.go:113 +0x611
@CLAassistant
Copy link

CLAassistant commented Dec 6, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 6, 2022
@hslatman hslatman self-requested a review December 7, 2022 05:13
@hslatman
Copy link
Member

hslatman commented Dec 7, 2022

Hey @jacostaperu, thank you for your report and your PR. I think a slightly different fix is needed, as the current patch may block multiple DNS names from being specified. I'll take a look soon.

@jacostaperu
Copy link
Author

I am adding DNSListValidate function to ensure Multiple DNS lists are accepted, also validating they are valid according to RFC-952 and RFC-1123

@jacostaperu jacostaperu closed this Dec 8, 2022
@jacostaperu jacostaperu reopened this Dec 8, 2022
@hslatman
Copy link
Member

Hey @jacostaperu, I noticed your proposed change didn't allow space-separated values (only comma-separated) and also didn't include IPv6 addresses, so we can't merge this as is. I've created a PR with a different fix for this specific issue: #815. It doesn't rely on regexes and fits better with the existing validation for this input. I think the additional validation of the hostname you created might still be useful, but may have to go into https://github.com/smallstep/cli-utils/blob/main/ui/validators.go.

@hslatman
Copy link
Member

Hey @jacostaperu, I'm closing this PR in favor of #815.

@hslatman hslatman closed this Jan 11, 2023
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.

3 participants