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

feat(server): Enforce TLS >= v1.2 #5172

Merged
merged 6 commits into from
Feb 24, 2021
Merged

feat(server): Enforce TLS >= v1.2 #5172

merged 6 commits into from
Feb 24, 2021

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Feb 23, 2021 •
edited
Loading

Before:

(⎈ |k3d-k3s-default:argo)➜  argo-workflows git:(tmp) curl -kv https://localhost:2746/workflows --tls-max 1.1
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 2746 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.1 (OUT), TLS handshake, Client hello (1):
* TLSv1.1 (IN), TLS handshake, Server hello (2):
* TLSv1.1 (IN), TLS handshake, Certificate (11):
* TLSv1.1 (IN), TLS handshake, Server key exchange (12):
* TLSv1.1 (IN), TLS handshake, Server finished (14):
* TLSv1.1 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.1 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.1 (OUT), TLS handshake, Finished (20):
* TLSv1.1 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.1 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.1 / ECDHE-RSA-AES256-SHA
* ALPN, server did not agree to a protocol
* Server certificate:
*  subject: CN=localhost; O=ArgoProj
*  start date: Feb 22 19:56:18 2021 GMT
*  expire date: Feb 22 19:56:18 2022 GMT
*  issuer: CN=localhost; O=ArgoProj
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
> GET /workflows HTTP/1.1
> Host: localhost:2746
> User-Agent: curl/7.64.1
> Accept: */*
> 
< HTTP/1.1 200 OK
...

After:

< 
(⎈ |k3d-k3s-default:argo)➜  argo-workflows git:(tmp) curl -kv https://localhost:2746/workflows --tls-max 1.1
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 2746 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.1 (OUT), TLS handshake, Client hello (1):
* TLSv1.1 (IN), TLS alert, protocol version (582):
* error:1400442E:SSL routines:CONNECT_CR_SRVR_HELLO:tlsv1 alert protocol version
* Closing connection 0
curl: (35) error:1400442E:SSL routines:CONNECT_CR_SRVR_HELLO:tlsv1 alert protocol version

@alexec
feat(argo-server): Enfore minimum TLS v1.2
8321854 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Merge branch 'master' into tls
8014f20 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
feat(argo-server): Enfore minimum TLS v1.2
7986d76 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec marked this pull request as ready for review February 23, 2021 20:40
whynowy
whynowy reviewed Feb 23, 2021
View reviewed changes
cmd/argo/commands/server.go
@@ -87,8 +88,13 @@ See %s`, help.ArgoSever),
if secure {
cer, err := tls.LoadX509KeyPair("argo-server.crt", "argo-server.key")
errors.CheckError(err)
// InsecureSkipVerify will not impact the TLS listener. It is needed for the server to speak to itself for GRPC.
tlsConfig = &tls.Config{Certificates: []tls.Certificate{cer}, InsecureSkipVerify: true}
tlsMinVersion, err := env.GetInt("TLS_MIN_VERSION", tls.VersionTLS12)
Copy link
Member

Choose a reason for hiding this comment

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

This ENV needs to be documented somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cmd/argo/commands/server.go
errors.CheckError(err)
tlsConfig = &tls.Config{
Certificates: []tls.Certificate{cer},
InsecureSkipVerify: false, // InsecureSkipVerify will not impact the TLS listener. It is needed for the server to speak to itself for GRPC.
Copy link
Member

Choose a reason for hiding this comment

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

Curious why changing to false still works for self-signed CERT.

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 think this is because HTTP connections loop back to GRPC, so we need, this

@alexec
feat(argo-server): Enfore minimum TLS v1.2
39ecd9a 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
feat(argo-server): Enfore minimum TLS v1.2
1b47d69 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@whynowy
Copy link
Member

whynowy commented Feb 24, 2021

codegen keeps failing due to

Documented variable TLS_MIN_VERSION in docs/environment-variables.md is not used anywhere
make: *** [codegen] Error 1
Error: Makefile:259: recipe for target 'codegen' failed

@alexec
feat(argo-server): Enfore minimum TLS v1.2
858a8ce 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Copy link
Contributor Author

alexec commented Feb 24, 2021

@whynowy fixed. Ready for review.

@alexec alexec changed the title feat(argo-server): Enforce minimum TLS v1.2 feat(server): Enforce minimum TLS v1.2 Feb 24, 2021
@alexec alexec changed the title feat(server): Enforce minimum TLS v1.2 feat(server): Enforce TLS >= v1.2 Feb 24, 2021
whynowy
whynowy approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

LGTM

@alexec alexec merged commit 199016a into argoproj:master Feb 24, 2021
@alexec alexec deleted the tls branch February 24, 2021 16:18
@alexec alexec added this to the v3.0 milestone Feb 24, 2021
@alexec
Copy link
Contributor Author

alexec commented Feb 24, 2021

@simster7 I think we should be avoiding back-porting features to v3.0 to now and only do bugs. This is an exception to that mind you.

@alexec alexec linked an issue Feb 25, 2021 that may be closed by this pull request
Enforce TLS v1.2 for Argo Server #5167
Closed
This was referenced Mar 1, 2021
Closed
Closed
alexec added a commit that referenced this pull request Mar 30, 2021
@alexec
feat(server): Enforce TLS >= v1.2 (#5172)
631e55d 
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec mentioned this pull request Mar 30, 2021
Closed
@simster7 simster7 mentioned this pull request Mar 31, 2021
Closed
77 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whynowy whynowy whynowy approved these changes

@jessesuen jessesuen Awaiting requested review from jessesuen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0
Development

Successfully merging this pull request may close these issues.

Enforce TLS v1.2 for Argo Server
2 participants
@alexec @whynowy