Implementation for ROLE_CERT_KEY_FILE_OUTPUT and its logic happens to be destructive.

[mlajkim]

Background

Outputting role cert key for role certificates feature has been implemented in https://github.com/AthenZ/k8s-athenz-sia/pull/38 but the default value for ROLE_CERT_KEY_FILE_OUTPUT is true, which it should be false

Also, it is not really straightforward for the code below and I believe it should simply output/not output based on the config itself. https://github.com/AthenZ/k8s-athenz-sia/blob/1c66fbc53936d9007dd6adecc88e5dba130ce5c3/pkg/identity/certificated.go#L107-L113

TODOs

• [x] Change the default ROLE_CERT_KEY_FILE_OUTPUT from true to false
• [x] Change the code here as the following instead.
  • [ ] Make sure the appropriate logger is handled too for this.

    // Much simpler and straightforward. (Does not require to understand `id == nil`)
    if idConfig.RoleCertKeyFileOutput {
    outKeyPath := filepath.Join(idConfig.RoleCertDir, rolecert.Domain+idConfig.RoleCertFilenameDelimiter+rolecert.Role+".key.pem")
    log.Debugf("Saving x509 role cert key[%d bytes] at [%s]", len(roleKeyPEM), outKeyPath)
    if err := w.AddBytes(outKeyPath, 0644, roleKeyPEM); err != nil {
    return errors.Wrap(err, "unable to save x509 role cert key")
    }
    }

• [x] Env is modified accordingly
• [x] ~Add the env for the Makefile too~
  • will be handled here instead in https://github.com/AthenZ/k8s-athenz-sia/issues/47

[WindzCUHK]

Also, it is not really straightforward for the code below and I believe it should simply output/not output based on the config itself.

Add warning message Rotate private key[<path>] will downtime in role certificate, please manually restart after rotation in configuration loading stage, if:

• external keyid == nil
• split key disabled idConfig.RoleCertKeyFileOutput
• but issue role certificate idConfig.RoleCertDir != "" && idConfig.TargetDomainRoles != ""

[mlajkim]

Add warning message Rotate private key[] will downtime in role certificate, please manually restart after rotation in configuration loading stage, if:

external keyid == nil split key disabled idConfig.RoleCertKeyFileOutput but issue role certificate idConfig.RoleCertDir != "" && idConfig.TargetDomainRoles != ""

Sounds like a plan!

Just letting you know that the code itself is within the role certificate loop, so the idConfig.RoleCertDir != "" && idConfig.TargetDomainRoles != "" is not really required to be checked.

And therefore you can just do the following (you should double check tho)

if idConfig.RoleCertKeyFileOutput {
  if id == nil {
     // Oopsy! No identity but no role cert key output enabled!
     // TODO: Write a log here
  } 
  outKeyPath := filepath.Join(idConfig.RoleCertDir, rolecert.Domain+idConfig.RoleCertFilenameDelimiter+rolecert.Role+".key.pem")
  log.Debugf("Saving x509 role cert key[%d bytes] at [%s]", len(roleKeyPEM), outKeyPath)
  if err := w.AddBytes(outKeyPath, 0644, roleKeyPEM); err != nil {
    return errors.Wrap(err, "unable to save x509 role cert key")
  }
}

[WindzCUHK]

Sorry for confusing. I mean the warning message and the checking should added in the below method. The condition idConfig.RoleCertKeyFileOutput for the split key block is good. https://github.com/AthenZ/k8s-athenz-sia/blob/c8478297a9d228ffc0a6a1ea469ad0ef8a682dc8/pkg/config/config.go#L222C33-L222C48

[mlajkim]

@WindzCUHK Actually logging inside the loop is a bad idea. MB.

Add warning message Rotate

private key[] will downtime in role certificate, please manually restart after rotation in configuration loading stage, if:

external keyid == nil split key disabled idConfig.RoleCertKeyFileOutput but issue role certificate idConfig.RoleCertDir != "" && idConfig.TargetDomainRoles != ""

I am technically not sure how you could implement external keyid == nil inside the validateAndInit() but if possible, I am down for it!

[WindzCUHK]

@mlajkim

Add the env for the Makefile too

I think there is no need to customize the default value in any cases. Is it better not to add the build argument?