openconfig / gnsi

Apache License 2.0
20 stars 18 forks source link

A more flexible abstraction for gNSI Certz #45

Closed brianneville closed 1 year ago

brianneville commented 2 years ago

Currently, gNSI certz assumes the following:

Target (as seen from gNSI.certificate microservice point of view)
 |
 +-+ certificate (used by all gRPC services)
 | +- certificate (with public key)
 | +- private key
 |
 +-+ trust bundle (Certificate Authority certificates)
 | +- CA Root certificate
 | +- CA Intermediate Certificate
 |
 +-+ Certificate Revocation List bundle
   +- Certificate Revocation List A
   +- Certificate Revocation List B

 The above shown topology implies that every operation performed using this
 service changes a credential for all gRPC services at the same time.

I would like to propose a different abstraction here, based on the idea of an SSL profile which encapsulates all of the certs/bundle/CRLs.

An SSL profile would have the same structure as above:

SSL profile
 |
 +-+ certificate
 | +- certificate (with public key)
 | +- private key
 |
 +-+ trust bundle (Certificate Authority certificates)
 | +- CA Root certificate
 | +- CA Intermediate Certificate
 |
 +-+ Certificate Revocation List bundle
   +- Certificate Revocation List A
   +- Certificate Revocation List B

The name of the profile would have to be specified through a new string field ssl_profile which is added in the RotateCertificateRequest type. If the ssl_profile field is left blank then the gNSI service should rotate the SSL profile which is currently in-use by the gNSI service.

An example of what this would look like in the certz.proto file:


// Request messages to rotate existing certificates on the target.
message RotateCertificateRequest {
  // If set to `true` the requested operation will succeed even if the
  // `version` is already in use (is not unique).
  // If set to `false` the requested operation will fail and the streaming RPC
  // will be closed with the `AlreadyExists` error if the `version` is already
  // in use (is not unique).
  // It is a no-op for the `finalize_rotation` request.
  bool force_overwrite = 1;

  // An identifier for the specific SSL profile 
  // (collection of certs/bundles/CRLs) 
  // which is being rotated through this stream
  // Leaving this field blank means that this stream
  // will rotate the SSL profile which is currently 
  // being used by the gNSI service.
  string ssl_profile = 2;

  // Request Messages.
  oneof rotate_request {
    GenerateCSRRequest generate_csr = 3;
    UploadRequest certificates = 4;
    FinalizeRequest finalize_rotation = 5;
  }
}

In the current proto spec, it is assumed that the system is pre-configured through the CLI so that all gRPC servers are using the same certs/bundles/CRLs.
For Arista systems, this means defining SSL profiles, assigning them names, then assigning these profiles to the gRPC servers that are started.

Compared to the current proto spec then, the only difference with these changes from the client's perspective would be that now they have to declare in their RotateCertificateRequest messages which SSL profile they are intending to rotate (or leave the field blank).

This brings advantage that multiple SSL profiles can be set up on the device and configured through gNSI.

marcushines commented 2 years ago

I do have concerns though on users trying to swap the "g* service certs" with other profiles

This would lead to confusion and require explicit configuration of services seperate from the servers they maybe implemented on

So if the use case is all g* services use a default profile and the ssl_profile is only relevant to "other servers or use cases like grpctunnel then i am good with that

@tomek-US @robshakir

tomek-US commented 2 years ago

The idea of having multiple sets of credentials for different purposes sounds tempting (and it was in a bit different form part of the previous version of this API) but I do not think we can simply add the profile name as a new field.

At least there would have to be a new API call added to list the available profiles.

Perhaps we would also need the Install() call back to create them. And a Delete() to remove them.

Also the question remains how to assign a profile to a service. I do not think that leaving the process unspecified is the best approach. In my opinion it calls for another API call(s) that would have to be able list available services and map the profiles to them. In theory using gNMI is another option but we might hit the 'chicken and egg' problem with this approach.

RonanMacF commented 2 years ago

Also the question remains how to assign a profile to a service.

This question is still an issue either way here. The main way for clients to associate a SSL profile with a service is through CLI. For gRPC services at least a leaf like this seems to have been made for this purpose over gNMI under /system/grpc-servers

   leaf certificate-id {
      type string;
      description
        "Name of the certificate that is associated with the gRPC service. The
        certificate ID is provisioned through other interfaces to the device, such
        as the gNOI certificate management service.";
    }

For users that don't want to have to think about this no SSL profile could mean the currently active profile so this would be transparent for users that don't want to care about it. Having an Install, list and Delete RPC also sounds like a good idea to me.

I'll need to think more on if there are other non gNMI/CLI ways to do this as it might be difficult to refer to a particular service through other means. I think gNMI way be the best solution here.

tomek-US commented 2 years ago

This leaf is marked as not supported here: https://github.com/openconfig/gnsi/blob/d96c5b33d6ba5290ea48f7a12fbd5475a09c0f41/certz/gnsi-certz.yang#L104 ;-)

RonanMacF commented 2 years ago

I'm not sure I agree with that? Imo new YANG models should only be additive, it should be left to the vendor to decide if they want to implement it or not.

Arista for example already implements this leaf. This means that we will not be able to pull updates from this repo easily as they are added as we will need to have our own forked copy which removes this deviation.

tomek-US commented 2 years ago

I have prepared a change that adds SSL profile management to gNSI.certz. Please take a look.

brianneville commented 1 year ago

Added a few comments on the PR there (relating to the use of repeated string ssl_profile_ids in some messages) but overall its looking good, thanks for working on this!