add signer:rootcert map into mesh config by irisdingbj · Pull Request #2090 · istio/api

irisdingbj · 2021-09-08T01:15:06Z

If istio acting as RA, user can specify signers for the CSR, We need a way to let user specify root cert for each of the signers. Detailed discussion can be found in: istio/istio#34486

This also opens doors for us to define signer to replace kubernetes.io/legacy-unknown for istiod certs, a related proposal is here

Another related proposal is here: https://docs.google.com/document/d/1rxTSLuepcE_OJHj4C23RMJcgyIZzlBI-GqO8Y37SPko/edit#heading=h.5l3l3tiit0zd , see Distribution of root of trust for signers section

irisdingbj · 2021-09-08T01:17:04Z

/test release-notes_api

rveerama1 · 2021-09-10T12:34:13Z

I think you have to run "make proto-commit" to update proto.lock with new changes.

irisdingbj · 2021-09-13T00:58:27Z

@myidpt @costinm @howardjohn @shankgan Please take a look at this PR to see whether it is on the right direction. Thanks!

irisdingbj · 2021-09-17T08:32:01Z

@myidpt @shankgan looking forward to your comments for this!

costinm · 2021-09-18T17:50:22Z

Storing public keys in secrets seems unnecessary and complex. We already have a dozen ways to handle public keys and CAs - can we extend one of the existing ones ?

…

On Sat, Sep 18, 2021, 07:54 John Howard ***@***.***> wrote: Assigned #2090 <#2090> to @costinm <https://github.com/costinm>. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#2090 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2WYNXJTMNC6W6546KDUCSR2JANCNFSM5DTQ3L2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

costinm · 2021-09-18T17:52:28Z

Also, why in meshconfig ? We have other ca settings in ProxyConfig - why would this be a global setting instead of per namespace/workload ?

…

On Fri, Sep 17, 2021, 11:06 shankgan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mesh/v1alpha1/config.proto <#2090 (comment)>: > @@ -877,8 +877,13 @@ message MeshConfig { // configured globally via this field. istio.networking.v1alpha3.HTTPRetry default_http_retry_policy = 62; + // Map of root certs for kubernetes CSR signers when Istiod is acting as RA(registration authority) + // Key is the signer name and value is the secretName which stores the root certs for the signer + // signer1: signer-root-cert.ns1 + map<string, string> signer_root_certificate = 63; nit: does this need to be a secret, as opposed to a configmap? Don't have any objection to using secret, just curious — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2090 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2WQERKIKR7E35FJRILUCN7SRANCNFSM5DTQ3L2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

irisdingbj · 2021-09-22T09:26:49Z

@costinm Thanks for the review!

The reason to add this into meshConfig instead of ProxyConfig is because

istiod need to validate the returned certs chain which are signed from different signers against the root certs specified in meshConfig
The cert for istiod itself can be signed via the specified signer as well.

I also went through the related fileds in meshConfig and have not found a good match for this task: sepcify root certs for different signers:

message CertificateData {
oneof certificate_data {
string pem = 1;
SPIFFE_Trust_Domain_and_Bundle.md#the-spiffe-trust-domain-and-bundle
string spiffe_bundle_url = 2;
}
}

this can not be used as it lack of places to specify signer name.

message CA {
// REQUIRED. Address of the CA server implementing the Istio CA gRPC API.
string address = 1;
istio.networking.v1alpha3.ClientTLSSettings tls_settings = 2;
google.protobuf.Duration request_timeout = 3;
bool istiod_side = 4;
}
this can not be used as address is a required filed but we do no need this.
message Certificate {
string secret_name = 1;
repeated string dns_names = 2;
}
this can not be used as it will use built-in signers and have no place to define signers.

howardjohn · 2021-09-22T22:36:42Z

If we are going to move forward with #1923 we may want consistency there. @mathetake @lizan is there still interest in that API?

If so, it may make sense to add two fields there: "trust_domains" for the use case in 1923 and "signers" for the use case in 1923.

mathetake · 2021-09-24T00:00:30Z

oh yeah we are still interested in 1923 so I would really appreciate it if we could leverage the API introduced here (trust_domains field).

irisdingbj · 2021-09-29T08:34:37Z

@howardjohn @shankgan PTAL to see the latest change which is reusing the certificate_data filed. @mathetake PTAL to see whether the adding for trust_domains is approriate or you want to take care of it in a separate PR.

Thanks!

mathetake

Thanks LGTM from me! (though I'm not sure if I can finish the multiple trust domains impl part for 1.12 as I am going into vacation for two weeks beginning the next week).

howardjohn · 2021-09-30T16:21:54Z

    }
+    // Optional. Specify the kubernetes signers to which this certificate data belongs to
+    // when Istiod is acting as RA(registration authority)
+    // If set, they are used for these signers. Otherwise, this root CA is used for default signer.


Suggested change

// If set, they are used for these signers. Otherwise, this root CA is used for default signer.

// If set, they are used for these signers. Otherwise, this root CA is used for all signers.

This enables a few things...:

Backwards compatibility preserved

You can opt into allowing 2 different signers to trust each other. This is critical for a migration between CAs

shankgan · 2021-09-30T17:38:37Z

      // The certificate is retrieved from the endpoint.
      string spiffe_bundle_url = 2;
    }
+    // Optional. Specify the kubernetes signers to which this certificate data belongs to


nit: "//Optional. Specify the kubernetes signers (External CA) that use this CA certificate" - think this sounds better

shankgan · 2021-09-30T17:42:02Z

+    // If set, they are used for these signers. Otherwise, this root CA is used for default signer.
+    repeated string cert_signers = 3;
+
+    // Optional. Specify the list of trust domains to which this certificate data belongs.


nit: could we replace the word certificate with trustAnchor in all the comments? This can be done in a later PR if needed.This entire field is used for CA certificates, that are used as trustAnchors within the mesh. The use of the word "certificate" is ambiguous IMO.

howardjohn

A few small comments to address but overall lgtm

irisdingbj · 2021-10-11T03:25:14Z

A few small comments to address but overall lgtm

Thanks! I addressed the comments and removed the on-hold label to get it in. Glad to address new comments in following up PR if needed.

irisdingbj requested review from brian-avery, howardjohn, linsun, louiscryan, nrjpoddar and smawson as code owners September 8, 2021 01:15

google-cla Bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 8, 2021

istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 8, 2021

irisdingbj added the release-notes-none Indicates a PR that does not require release notes. label Sep 8, 2021

rveerama1 reviewed Sep 10, 2021

View reviewed changes

Comment thread mesh/v1alpha1/config.proto

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 13, 2021

JoshVanL reviewed Sep 14, 2021

View reviewed changes

Comment thread mesh/v1alpha1/config.proto Outdated

shankgan approved these changes Sep 17, 2021

View reviewed changes

howardjohn assigned costinm Sep 18, 2021

mathetake reviewed Sep 24, 2021

View reviewed changes

Comment thread mesh/v1alpha1/config.proto Outdated

irisdingbj force-pushed the signer_root_cert branch from 5813226 to af5b97a Compare September 29, 2021 08:31

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 29, 2021

howardjohn reviewed Sep 29, 2021

View reviewed changes

Comment thread mesh/v1alpha1/config.proto Outdated

mathetake approved these changes Sep 30, 2021

View reviewed changes

howardjohn reviewed Sep 30, 2021

View reviewed changes

shankgan reviewed Sep 30, 2021

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 2, 2021

howardjohn approved these changes Oct 4, 2021

View reviewed changes

howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Oct 4, 2021

add signer and trustdomains into certificateData

306fa26

irisdingbj force-pushed the signer_root_cert branch from ff4ad5c to 306fa26 Compare October 11, 2021 03:09

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2021

irisdingbj removed the do-not-merge/hold Block automatic merging of a PR. label Oct 11, 2021

istio-testing merged commit cc56825 into istio:master Oct 11, 2021

This was referenced Oct 19, 2021

Add SPIFFE independent multiple trust domain API. #1923

Closed

Support multiple trust domains in trustbundle pkg. istio/istio#35680

Closed

	// If set, they are used for these signers. Otherwise, this root CA is used for default signer.
	// If set, they are used for these signers. Otherwise, this root CA is used for all signers.

Conversation

irisdingbj commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

irisdingbj commented Sep 8, 2021

Uh oh!

Uh oh!

rveerama1 commented Sep 10, 2021

Uh oh!

irisdingbj commented Sep 13, 2021

Uh oh!

Uh oh!

irisdingbj commented Sep 17, 2021

Uh oh!

costinm commented Sep 18, 2021 via email

Uh oh!

costinm commented Sep 18, 2021 via email

Uh oh!

irisdingbj commented Sep 22, 2021

Uh oh!

howardjohn commented Sep 22, 2021

Uh oh!

mathetake commented Sep 24, 2021

Uh oh!

Uh oh!

irisdingbj commented Sep 29, 2021

Uh oh!

Uh oh!

mathetake left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

irisdingbj Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

shankgan Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irisdingbj Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

shankgan Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irisdingbj Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

irisdingbj commented Oct 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

irisdingbj commented Sep 8, 2021 •

edited

Loading

shankgan Sep 30, 2021 •

edited

Loading

shankgan Sep 30, 2021 •

edited

Loading