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

Auto encrypt k8s #6944

Merged
merged 6 commits into from
Jan 17, 2020
Merged

Auto encrypt k8s #6944

merged 6 commits into from
Jan 17, 2020

Conversation

hanshasselberg
Copy link
Member

@hanshasselberg hanshasselberg commented Dec 13, 2019

Since auto_encrypt certs are being used to serve HTTPS (#6489) they should have sensible Subject Alternative Name set instead of only providing the spiffe id. And there is a way to configure additional DNSSAN and IPSAN which will be used by the agent when requesting auto_encrypt certs. Note the following cert, which has consul.io and 192.168.1.1 set.

$ echo | \
                       openssl s_client -connect localhost:8501 2>/dev/null | \
                       openssl x509 -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 11 (0xb)
    Signature Algorithm: ecdsa-with-SHA256
        Issuer: CN=pri-9ioo0o5x.consul.ca.c1ce7653.consul
        Validity
            Not Before: Jan 16 10:50:46 2020 GMT
            Not After : Jan 19 10:50:46 2020 GMT
        Subject: CN=c1.agnt.c1ce7653.consul
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:48:da:ae:28:fc:b5:2e:f6:63:d3:0b:58:29:e6:
                    71:d4:1f:ab:f9:df:e5:bc:e6:79:be:6a:05:97:92:
                    95:34:c0:0f:d6:91:79:bc:67:e8:db:7b:bb:0a:1c:
                    1e:a9:e8:c8:de:2a:b6:ed:66:64:40:3d:38:c2:3f:
                    d0:63:6b:c9:b9
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        X509v3 extensions:
            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment, Data Encipherment, Key Agreement
            X509v3 Extended Key Usage:
                TLS Web Client Authentication, TLS Web Server Authentication
            X509v3 Basic Constraints: critical
                CA:FALSE
            X509v3 Subject Key Identifier:
                F3:A0:08:64:C7:4F:FC:D6:64:33:92:93:C8:6E:65:A9:C0:B0:D7:74:3D:95:D0:1C:1E:50:18:FB:9A:BE:B3:F9
            X509v3 Authority Key Identifier:
                keyid:F3:A0:08:64:C7:4F:FC:D6:64:33:92:93:C8:6E:65:A9:C0:B0:D7:74:3D:95:D0:1C:1E:50:18:FB:9A:BE:B3:F9

            X509v3 Subject Alternative Name:
                DNS:localhost, DNS:consul.io, IP Address:127.0.0.1, IP Address:0:0:0:0:0:0:0:0, IP Address:192.168.1.1, URI:spiffe://c1ce7653-457a-7942-f7a3-fedb029b7fc2.consul/agent/client/dc/dc1/id/c1
    Signature Algorithm: ecdsa-with-SHA256
         30:45:02:21:00:b6:11:b7:96:ef:98:ca:55:f1:73:f7:1d:a1:
         ad:07:ba:d8:2c:41:c9:67:9c:0f:ca:88:15:40:e7:ee:93:23:
         72:02:20:09:f5:27:f0:df:f9:fc:02:a8:40:ef:06:e6:28:b5:
         35:b3:22:1d:50:27:75:a0:6b:c4:36:30:bc:de:bc:bb:00

Todo:

  • make additional SAN configurable
  • check if the vault and aws pca correctly assigns SAN

@codecov
Copy link

codecov bot commented Dec 13, 2019

Codecov Report

Merging #6944 into master will increase coverage by 0.27%.
The diff coverage is 53.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6944      +/-   ##
==========================================
+ Coverage   65.35%   65.62%   +0.27%     
==========================================
  Files         447      443       -4     
  Lines       53603    53304     -299     
==========================================
- Hits        35031    34981      -50     
+ Misses      14362    14105     -257     
- Partials     4210     4218       +8
Impacted Files Coverage Δ
agent/connect/csr.go 16.66% <0%> (-1.52%) ⬇️
agent/consul/auto_encrypt.go 50.92% <100%> (+0.92%) ⬆️
agent/connect/ca/provider_consul.go 59.51% <100%> (+0.54%) ⬆️
agent/cache-types/connect_ca_leaf.go 72.8% <60%> (-0.41%) ⬇️
agent/consul/state/index_service_kind.go 25% <0%> (-25%) ⬇️
agent/consul/raft_rpc.go 78.72% <0%> (-4.26%) ⬇️
agent/consul/session_ttl.go 85.48% <0%> (-3.23%) ⬇️
agent/consul/replication.go 85.71% <0%> (-2.39%) ⬇️
agent/consul/intention_endpoint.go 67.68% <0%> (-2.22%) ⬇️
agent/http_oss.go 50% <0%> (-2.18%) ⬇️
... and 61 more

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 8673bc2...6e2f4f9. Read the comment docs.

Copy link
Member

@lornasong lornasong left a comment

Choose a reason for hiding this comment

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

thanks for sharing your PR, this makes sense to me!

@hanshasselberg hanshasselberg self-assigned this Jan 6, 2020
@hanshasselberg hanshasselberg force-pushed the auto_encrypt_k8s branch 2 times, most recently from 72f5a0a to e7ed588 Compare January 16, 2020 10:48
@hanshasselberg hanshasselberg requested review from lornasong and a team January 16, 2020 10:52
@hanshasselberg hanshasselberg force-pushed the auto_encrypt_k8s branch 2 times, most recently from 7df318c to 45a6c90 Compare January 16, 2020 12:23
Copy link
Member

@lornasong lornasong left a comment

Choose a reason for hiding this comment

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

looks good to me! only left suggestion of places to add comments inline

agent/config/config.go Outdated Show resolved Hide resolved
agent/config/runtime.go Outdated Show resolved Hide resolved
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me, made a couple suggestions inline but they're not blocking.

agent/connect/csr.go Outdated Show resolved Hide resolved
extensions ...pkix.Extension) (string, error) {
return CreateCSRWithSAN(uri, commonName, privateKey, nil, nil, extensions...)
}

// CreateCSR returns a CA CSR to sign the given service along with the PEM-encoded
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// CreateCSR returns a CA CSR to sign the given service along with the PEM-encoded
// CreateCACSR returns a CA CSR to sign the given service along with the PEM-encoded

@hanshasselberg hanshasselberg merged commit 87f32c8 into master Jan 17, 2020
@hanshasselberg hanshasselberg deleted the auto_encrypt_k8s branch January 17, 2020 22:25
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.

3 participants