feat: add HTTPS to dex server (#9424)

[notfromstatefarm]

Signed-off-by: notfromstatefarm [email protected]

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

Closes #9424

This PR implements TLS for the dex server in a nearly identical fashion to the repo server TLS, using the argocd-dex-server-tls secret and falling back to a generated self-signed certificate if it is not present.

By default, TLS will now be enabled for communications between the API server and repo server. However, just like with the repo server, it's best to use a user-provided certificate. The API server will validate the certificate against the secret.

Thanks @crenshaw-dev for pointing this issue out to me! 🥳

dex time="2022-07-06T04:46:06Z" level=info msg="ArgoCD Dex Server is starting" built="2022-07-06T04:44:25Z" commit=1f78d1708b972c60423ca3710a655ea413b006b
e namespace=argocd version=v2.4.0+1f78d17.dirty
dex time="2022-07-06T04:46:06Z" level=info msg="Loading TLS configuration from cert=/tls/tls.crt and key=/tls/tls.key"
...
dex time="2022-07-06T04:46:06Z" level=info msg="listening (https) on 0.0.0.0:5556"
dex time="2022-07-06T04:46:17Z" level=info msg="login successful: connector \"github\", username=\"Jake\", preferred_username=\"notfromstatefarm\", ...

[codecov]

Codecov Report

Merging #9883 (7171723) into master (b515ea7) will decrease coverage by 0.01%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           master    #9883      +/-   ##
==========================================
- Coverage   45.97%   45.96%   -0.02%     
==========================================
  Files         227      227              
  Lines       27230    27276      +46     
==========================================
+ Hits        12520    12538      +18     
- Misses      13011    13036      +25     
- Partials     1699     1702       +3     

Impacted Files	Coverage Δ
util/dex/dex.go	57.14% <16.66%> (-30.74%)	⬇️
util/tls/tls.go	74.66% <23.07%> (-3.54%)	⬇️
util/dex/config.go	85.56% <50.00%> (-3.45%)	⬇️
server/server.go	51.49% <60.00%> (+0.24%)	⬆️
util/oidc/oidc.go	56.12% <100.00%> (+0.34%)	⬆️
util/session/sessionmanager.go	73.94% <100.00%> (+0.20%)	⬆️
util/jwt/jwt.go	63.93% <0.00%> (ø)
util/settings/settings.go	50.90% <0.00%> (ø)
util/grpc/logging.go	58.33% <0.00%> (+18.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b515ea7...7171723. Read the comment docs.

[notfromstatefarm]

In case anyone wants to test out strict validation/providing your own cert super quickly, cert-manager can be installed and configured in two steps:

kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.8.2/cert-manager.yaml

and then apply this manifest:

apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: selfsigned-issuer
spec:
  selfSigned: {}
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: argocd-ca
  namespace: argocd
spec:
  isCA: true
  commonName: argocd-ca
  secretName: root-secret
  privateKey:
    algorithm: ECDSA
    size: 256
  issuerRef:
    name: selfsigned-issuer
    kind: ClusterIssuer
    group: cert-manager.io
---
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
  name: argocd-issuer
  namespace: argocd
spec:
  ca:
    secretName: root-secret
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: argocd-dex-server
  namespace: argocd
spec:
  secretName: argocd-dex-server-tls
  issuerRef:
    name: argocd-issuer
  commonName: argocd-dex-server
  dnsNames:
  - argocd-dex-server

After this PR I'm going to take a look at auto-reloading whenever the TLS changes. As of right now, repo server and dex server both have to be restarted manually if the TLS changes, which prevents effective use of cert-manager.

[notfromstatefarm]

something funky is going on with CI and the tests are failing with nonsensical errors.. lets see if a new commit helps any.

[crenshaw-dev]

Just tested locally, works like a charm. Thanks so much again!

I'm going to look for a second review, since this is very core security work.

[notfromstatefarm]

the latest GHSA merge broke some of the tests, figuring it out!

[notfromstatefarm]

okay, got it all figured out. Long story short, the GHSA merge introduced a new OIDCTLSInsecureSkipVerify setting that is ignored by this PR. Had to update the tests to use the dexConfigTls for the internal dex tests.

Might need to document that we're ignoring the OIDC TLS settings when using internal dex.

[crenshaw-dev]

@notfromstatefarm how would you feel about dropping the Dex-specific TLS verification flag in favor of the one introduced by the GHSA? Is there any advantage to having a setting specific to Dex?

[notfromstatefarm]

@notfromstatefarm how would you feel about dropping the Dex-specific TLS verification flag in favor of the one introduced by the GHSA? Is there any advantage to having a setting specific to Dex?

There's a few problems with that off the top of my head

1. We're matching the logic used for connecting to the repo-server, which means the same flags (--xxx-server-plaintext and --xxx-server-strict-tls). It would be confusing if the same type of functionality was configured in two different ways, whereas we can clearly define internal versus external OIDC configurations.
2. The GHSA OIDC configuration simply defines 'strict validation on or off', it does not define HTTPS on/off like the repo-server and dex-server config does.
3. The default is that strict validation is on for external OIDC, whereas it needs to be off by default for the internal dex server

[crenshaw-dev]

@notfromstatefarm that makes sense! My brain was quite tired last night. :-)