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

Login must ignore some SSL cert errors when --insecure #11145

Merged
merged 1 commit into from
Sep 30, 2016
Merged

Login must ignore some SSL cert errors when --insecure #11145

merged 1 commit into from
Sep 30, 2016

Conversation

fabianofranz
Copy link
Member

Fixes #11122

When oc login --insecure-skip-tls-verify some SSL cert errors like hostname mismatch, cert invalid or expired, etc must be ignored.

@thoraxe @liggitt @openshift/cli-review

@fabianofranz
Copy link
Member Author

[test]

liggitt
liggitt suggested changes Sep 28, 2016
View reviewed changes
pkg/cmd/cli/cmd/login/loginoptions.go
@@ -121,7 +121,7 @@ func (o *LoginOptions) getClientConfig() (*restclient.Config, error) {
// certificate authority unknown, check or prompt if we want an insecure
// connection or if we already have a cluster stanza that tells us to
// connect to this particular server insecurely
case x509.UnknownAuthorityError:
case x509.UnknownAuthorityError, x509.HostnameError, x509.CertificateInvalidError:
if o.InsecureTLS ||
hasExistingInsecureCluster(*clientConfig, *o.StartingKubeConfig) ||
promptForInsecureTLS(o.Reader, o.Out) {
Copy link
Contributor

@liggitt liggitt Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

promptForInsecureTLS needs to tweak the text based on the error... pass in the error, and switch inside promptForInsecureTLS on the error type

Copy link
Member Author

Choose a reason for hiding this comment

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

@liggitt Sounds good, should we also show the original error message when prompting (so that in case of HostnameError it displays the expected/actual hostnames), or would a generic message be enough?

Copy link
Contributor

@liggitt liggitt Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I would show the original error for hostname and certificateinvalid errors (along with nice wrapping text)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Just note that it will prompt and in case you say n, it will error out and exit with pretty much the same message, just adding the error: prefix.

pkg/cmd/util/clientcmd/errors.go
@@ -50,6 +53,12 @@ func GetPrettyMessageForServer(err error, serverName string) string {
serverName = "server"
}
return fmt.Sprintf(tlsOversizedRecordMsg, err, serverName)

case certificateHostnameErrorReason:
return fmt.Sprintf("The set of authorized names doesn't match the requested name: %s", err)
Copy link
Contributor

@liggitt liggitt Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The server is using a certificate that does not match its hostname: %s?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, done.

pkg/cmd/util/clientcmd/errors.go
return fmt.Sprintf("The set of authorized names doesn't match the requested name: %s", err)

case certificateInvalidReason:
return fmt.Sprintf("Invalid certificate: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The server is using an invalid certificate: %s

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@fabianofranz
Copy link
Member Author

@liggitt comments addressed.

@fabianofranz fabianofranz force-pushed the issues_11122 branch 2 times, most recently from 362d350 to 18a0307 Compare September 28, 2016 21:11
liggitt
liggitt suggested changes Sep 28, 2016
View reviewed changes
Copy link
Contributor

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

a couple tweaks, then squash and LGTM

pkg/cmd/cli/cmd/login/helpers.go
@@ -80,10 +81,23 @@ func dialToServer(clientConfig restclient.Config) error {
return nil
}

func promptForInsecureTLS(reader io.Reader, out io.Writer) bool {
func promptForInsecureTLS(reader io.Reader, out io.Writer, err error) bool {
var insecureTLSRequestReason string
Copy link
Contributor

Choose a reason for hiding this comment

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

probably need a generic default reason

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think so, we are already filtered on the errors we check. If in future this changes or gets called by something else, you are asking for "prompt for insecure" when calling this anyway, and the reason line will just be suppressed.

pkg/cmd/util/clientcmd/errors.go
@@ -103,6 +123,12 @@ func detectReason(err error) int {
case strings.Contains(err.Error(), "tls: oversized record received"):
return tlsOversizedRecordReason
}
switch err.(type) {
Copy link
Contributor

@liggitt liggitt Sep 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

go ahead and add a case for UnknownAuthorityError here and return certificateAuthorityUnknownReason, just in case they change the text in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@fabianofranz fabianofranz force-pushed the issues_11122 branch 2 times, most recently from 4348669 to 23c7cd6 Compare September 28, 2016 22:01
@fabianofranz
Copy link
Member Author

@liggitt should we backport?

@liggitt
Copy link
Contributor

liggitt commented Sep 29, 2016 

--- FAIL: TestTLSWithInvalidCertificate (0.38s)
    loginoptions_test.go:125: evaluating test: succeed skipping tls
    loginoptions_test.go:125: evaluating test: certificate name doesn't match
    loginoptions_test.go:134: certificate name doesn't match: expected error "The set of authorized names doesn't match the requested name" but got "The server is using a certificate that does not match its hostname: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs"

@fabianofranz
Copy link
Member Author

@liggitt yep, fixed.
Could be a separate PR but I think I'll add a few more tests with generated certs, like one for expired, etc.

@fabianofranz
Copy link
Member Author

[merge]

@juanvallejo juanvallejo mentioned this pull request Sep 29, 2016
Closed
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 34a5bfd

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9517/)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 34a5bfd

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 30, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9540/) (Image: devenv-rhel7_5108)

@openshift-bot openshift-bot merged commit 23b6f26 into openshift:master Sep 30, 2016
@fabianofranz fabianofranz mentioned this pull request Sep 30, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt 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.
3 participants
@fabianofranz @liggitt @openshift-bot