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

cmd/tailscale: disable HTTPS verification for QNAP auth. #6919

Merged
merged 1 commit into from Jan 11, 2023
Merged

Conversation

DentonGentry
Copy link
Contributor

QNAP's "Force HTTPS" mode redirects even localhost HTTP to HTTPS, but uses a self-signed certificate which fails verification. We accommodate this by disabling checking of the cert.

Fixes #6903

Signed-off-by: Denton Gentry dgentry@tailscale.com

// QNAP Force HTTPS mode uses a self-signed certificate.
// https://github.com/tailscale/tailscale/issues/6903
tr := &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a well-meaning bot. The point of this PR is that QNAP's feature forces us to use HTTPS and then ignore the certificate it presents.

I'm looking for a way to add a comment or otherwise suppress this finding, we don't want it firing every time someone modifies this file.


From github/codeql#7937 it looks like there isn't a way to do this inline using code comments.
github/codeql-action#1098 provides a way to modify the GitHub Action.
Update: github/codeql#4786 implies that it does not work for Go.

@@ -247,7 +247,13 @@ func qnapAuthnSid(r *http.Request, user, sid string) (string, *qnapAuthResponse,
}

func qnapAuthnFinish(user, url string) (string, *qnapAuthResponse, error) {
resp, err := http.Get(url)
// QNAP Force HTTPS mode uses a self-signed certificate.
// https://github.com/tailscale/tailscale/issues/6903
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we somehow discover the cert and use that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It currently lives in /etc/ssl/certs/myhost.crt (and another copy of itself in /etc/ssl/certs/myrootca.crt to sign itself with).

I was concerned about the fragility in case QNAP ever moves it, but I suppose it isn't likely to move outside of /etc/ssl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do this, by importing the QNAP root CA:

+func qnapGetInsecureTLSConfig() *tls.Config {
+       return &tls.Config{InsecureSkipVerify: true}
+}
+
+func qnapGetTLSConfig() *tls.Config {
+       rootCAs, err := x509.SystemCertPool()
+       if err != nil {
+               log.Printf("qnapGetTLSConfig SystemCertPool failed: %v, falling back to no verification", err)
+               return qnapGetInsecureTLSConfig()
+       }
+
+       pem, err := os.ReadFile("/etc/ssl/certs/myrootca.crt")
+       if err != nil {
+               log.Printf("qnapGetTLSConfig read myrootca.crt: %v, falling back to no verification", err)
+               return qnapGetInsecureTLSConfig()
+       }
+
+       if !rootCAs.AppendCertsFromPEM(pem) {
+               log.Println("qnapGetTLSConfig AppendCertsFromPEM failed, falling back to no verification", err)
+               return qnapGetInsecureTLSConfig()
+       }
+
+       return &tls.Config{RootCAs: rootCAs}
+}
+
 func qnapAuthnFinish(user, url string) (string, *qnapAuthResponse, error) {
        // QNAP Force HTTPS mode uses a self-signed certificate.
        // https://github.com/tailscale/tailscale/issues/6903
        tr := &http.Transport{
-               TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+               TLSClientConfig: qnapGetTLSConfig(),
        }
        client := &http.Client{Transport: tr}
        resp, err := client.Get(url)

However this still doesn't work because though it is now signed by a valid CA, none of the CNs match and there are no alternative names:

Get "https://127.0.0.1:443/cgi-bin/authLogin.cgi?sid=jvys7c89": x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs

The fields in the certificate are:

emailAddress = support@qnap.com
CN = QNAP NAS
OU = QTS
O = QNAP Systems, Inc.
L = Taipei
ST = Taipei
C = TW

The CN is not a valid hostname, and there are no alternate names.
I don't think this certificate can pass any validation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for testing. Can you leave a comment describing this in the code?

QNAP's "Force HTTPS" mode redirects even localhost HTTP to
HTTPS, but uses a self-signed certificate which fails
verification. We accommodate this by disabling checking
of the cert.

Fixes #6903

Signed-off-by: Denton Gentry <dgentry@tailscale.com>
@DentonGentry
Copy link
Contributor Author

This will be delivered in a future 1.36.0 release for QNAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QNAP: cannot authenticate if "Force HTTPS" enabled in General Settings
2 participants