tink-crypto / tink

Tink is a multi-language, cross-platform, open source library that provides cryptographic APIs that are secure, easy to use correctly, and hard(er) to misuse.
https://developers.google.com/tink
Apache License 2.0
13.47k stars 1.18k forks source link

Add Destroy Method to Go Manager #628

Closed theory closed 1 year ago

theory commented 2 years ago

Should there be a Destory method on the Go manager? Something like:

// Destroy will destroy the key with given keyID.
// Returns an error if the key is not found or it is the primary key.
func (km *Manager) Destroy(keyID uint32) error {
    if km.ks == nil {
        return errors.New("keyset_manager: cannot destroy key, no keyset")
    }
    if km.ks.PrimaryKeyId == keyID {
        return errors.New("keyset_manager: cannot destroy the primary key")
    }
    for i, key := range km.ks.Key {
        if key.KeyId != keyID {
            continue
        }
        if key.Status == tinkpb.KeyStatusType_ENABLED || key.Status == tinkpb.KeyStatusType_DESTROYED {
            km.ks.Key[i].Status = tinkpb.KeyStatusType_DESTROYED
            return nil
        }
        return fmt.Errorf("keyset_manager: cannot destroy key with id %d with status %s", keyID, key.Status.String())
    }
    return fmt.Errorf("keyset_manager: key with id %d not found", keyID)
}
juergw commented 1 year ago

In Java, we now have a new API to edit KeysetHandles, namely the KeysetHandle.Builder. We want to move away from the KeysetManager.

In golang, we don't yet have this new API, but eventually we also want to move away from keyset.Manager. So we prefer to not put any additional work into this API.

Why do you need Destroy? Couldn't you use Delete instead?

theory commented 1 year ago

I only noted it here because it was missing and therefore diverged from the docs. I don't use it.

juergw commented 1 year ago

Ok, thanks. Which docs are you referring to? If the documentation says something different than the code then we should update the documentation.

theory commented 1 year ago

Oh, maybe I didn't see it in the docs, but in the tinkey CLI:

❯ tinkey                                                  
Argument wrong!
org.kohsuke.args4j.CmdLineException: Argument "command" is required

 [add-key | convert-keyset |            : Command to run
 create-keyset | create-public-keyset      
 | delete-key | destroy-key |              
 disable-key | enable-key |                
 list-keyset | list-key-templates |        
 rotate-keyset | promote-key]     

I was implementing a tinkey-like CLI so was looking for completeness. I do use use delete, and honestly wasn't sure what the difference was supposed to be.

tholenst commented 1 year ago

Thanks for filing this, but we believe this whole DESTROYED thing is rather confusing. We will probably solve this differently and most likely remove "destroyed". I will close this.