Fix step 9 to specifically create a seperate ssl profile from

[marcushines]

the system default. This aligns with certz description also for vendor implementations which disallow the modification of the vendor provided default

[coveralls]

Pull Request Test Coverage Report for Build 8287968367

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 88.45%

---

Totals
Change from base Build 8287476575:	0.0%
Covered Lines:	291
Relevant Lines:	329

---

💛 - Coveralls

[jenia-grunin]

Please address README wording, proto comments and bazel version.

[jenia-grunin]

Other than a small typo, looks good to me! As I mentioned in #36 (comment), please kick off the next action items internally and with vendors.

[LimeHat]

Note that this may not be necessary.
Another way to model an iDevID/oiDevID-based profile is to introduce a flag/enum to the tls-profile configuration.

E.g. a profile can be modelled as

profile Bob {
certificate  
key  
trust_bundle
...
enum use_devid
}

and use_devid enum can have three values (disable, oiDevid, iDevID); or it can be expressed as bool true/false (if true, oiDevID will be used when available, iDevID otherwise)

In that case there's no need to specify the profile names explicitly, you can say that all profiles with the flag should use the updated cert data.

[brianneville]

In that case there's no need to specify the profile names explicitly, you can say that all profiles with the flag should use the updated cert data.

I think we should be consistent - if we're using certz for SSL profile management, and certz operates on SSL profiles, then we should avoid creating some alternative representation of these profiles with extra attestz-specific flags.

Unless you were considering adding this into the certz spec too, in which case im still not really sure its a good approach :)

Fundamentally the SSL profile is just a logical grouping of files (certs/keys/crls) which can be applied to a gRPC server. If we attach some additional flag to these files which specifies that "updates to this profile affect all other profiles" then we have this kind of transitive dependency which isnt immediately visible to the client performing a certz rotation.
I.e. an update to SSL profile A now may have knock-on effects for some unknown SSL profile B and C

[marcushines]

I agree with Brian here - it actually makes it a bit confusing on intent if you add that enum - you can't use both oidevid and a cert in the profile?

In presenting this PR to the OC forum yesterday they felt the design was straightforward but happy to discuss.

[morrowc]

might this conversation (all of it really) benefit from a video-chat to hash out the whys/where-fores?

[LimeHat]

Hi @brianneville
There are a few things.

1. certZ is not involved in attestz/enrollz process

2. then we should avoid creating some alternative representation of these profiles with extra attestz-specific flags

The flags are not attestz-specific per se, and can be used with any profile management protocol. On the contrary, our proposal is to add an OpenConfig-compliant way to represent a tls-profile that uses iDevID or oiDevID.

To keep things very simple, we can start with a question: how would you represent an idevID-based profile in OC today? There's no obvious way, except for the previous proposal with a fixed name (that only the default profile is based on iDevID and it has a specific name).

4. Unless you were considering adding this into the certz spec too, in which case im still not really sure its a good approach :)

That would be a logical extension of this proposal, yes. Can be done in a backward-compatible way (since the default is disable or false).

If we attach some additional flag to these files which specifies that "updates to this profile affect all other profiles" then we have this kind of transitive dependency which isnt immediately visible to the client performing a certz rotation.
I.e. an update to SSL profile A now may have knock-on effects for some unknown SSL profile B and C

The key point is that it is not a regular certZ rotation; certZ is not involved here. Also, it depends on how you look at the oiDEVid-based profiles.
I don't believe that an operator expects to maintain multiple oiDevID-based certificates on a single node and use different oiDevID-based certificates with different profiles.

@morrowc , i would agree, a meeting could be helpful

[LimeHat]

regarding the previous comment, note that the custom address scenario is not applicable to your use case, at least for now. If we do a generic enum, it can be extended later to support this use case.

@brianneville indeed our thinking is about a general modelling of tls profiles, not directly related to any interface (although this conversation was initiated due to a specific workflow).
one thing that we need to consider is that tpm storage can be applicable not only to keys, but to certificates as well (in idevid/oidevid scenarios specifically). hence the enum should not be limited to the key field.

[brianneville]

one thing that we need to consider is that tpm storage can be applicable not only to keys, but to certificates as well (in idevid/oidevid scenarios specifically). hence the enum should not be limited to the key field.

Maybe we could capture this in the Certificate message type like this:

message Certificate {
  // Type of certificate.
  CertificateType type = 1;
  // Certificate encoding type.
  CertificateEncoding encoding = 2;

  // Actual certificate.
  // The exact encoding depends upon the type of certificate.
  // for X509, this should be a PEM encoded Certificate.
  bytes certificate = 3;
  
  // Optional.
  // Not needed if the device knows the private key or does not have to know it.
  bytes private_key = 4;
  
  enum KeySource {
     KEYSOURCE_UNSPECIFIED = 0;
     KEYSOURCE_GENERATED = 1; // case where certz generated the key with a CSR
     KEYSOURCE_TRANSFER = 2; // case where key is transferred in this message ( private_key != nil )
     
     // keys corresponding to specific certs
     KEYSOURCE_OIDEVID_TPM = 3; 
     KEYSOURCE_IDEVID_TPM = 4; 
  }
  
  KeySource key_source = 5;

  enum CertSource {
     CERTSOURCE_UNSPECIFIED = 0;
     CERTSOURCE_TRANSFER = 1; // case where cert is transferred in this message ( certificate != nil )
     
     CERTOURCE_OIDEVID_TPM = 3; // cert is oidevid stored in TPM
     CERTOURCE_IDEVID_TPM = 4; // cert is idevid stored in TPM
  }
  
  CertSource cert_source = 6;
}

[marcushines]

i think this is where we are headed
though might use a oneof rather than the enum and + certi/ key fields

oneof i believe would make this simpler code wise

[brianneville]

sounds good to me!

[marcushines]

openconfig/gnsi#172

[jenia-grunin]

It looks like we have converged on the changes in this PR and the remaining open items are related to certz repo, correct? If so, how about we merge this PR and continue iterating in a separate PR sent to certz repo? If all vendors could also please click approve button on this PR, that would also be very helpful :) The changes look good to me, so I am happy to approve once all vendors confirm that these changes are sensible/feasible to support.

[LimeHat]

The addition of ssl_profile_id in EnrollZ is ok with us. Could you please also validate the entire updated workflow described in b/318019628#comment19 so we can proceed with the development?