Add support for Vault Transit Secret Engine

[vsevel]

Fixes #4888.

From #4888:
The transit engine provides encryption as a service for a client application. Instead of storing your encryption key securely, and retrieve it from your application to apply the encrypt/decrypt algorithm locally, you delegate this to vault: You just ask vault to create a key and associate it with a name that you pass, then use the encrypt and decrypt services from vault passing just the key name. That way you do not have to embed the encryption algorithms in your app, and the encryption key never leaves the vault.
Encrypting/Decrypting sensitive data that need to be stored in a primary store is a very common use case. This would bring value to quarkus to make that feature available and usable with ease.

[vsevel]

@sberyozkin

• VaultTransitContext nonce and context: As a user, there is probably no way I'm going to start generating the nonces in my code, to ensure they are securely random, etc, perhaps it is really a job for the engine itself to be optionally activated by the user configuration (generate-nonce = true, etc) ? Of course the users can do it too, but I'm not sure they should be done by them;

I am not too familiar with the use of nonce (beside the doc).
there is an interesting section about convergent encryption.
basically the v1 algorithm was valid for keys generated in vault 0.6.1. v2 was susceptible to brute force decryption. and v3 is the current one.
the nonce attribute in the encrypt data documentation says This must be provided if convergent encryption is enabled for this key and the key was generated with Vault 0.6.1. Not required for keys created in 0.6.2+.
so it looks like something of the past.
I was actually hesitant to add support for it, as my understanding is that if you generated your keys after 0.6.1, then you should not use it.
if we leave it, may be we should not try to outsmart it, and let the users specify it themselves.

and it is not clear what the context[] is - something from the current client request ?

not sure what you mean

• 2 encrypt functions (one taking a byte[] and another one taking the list of batch items) seem to duplicate each other (same for sign...), as the former can be done with the latter (with either an empty of single list containing a byte[] and context) but may be I've missed something

Basically all data is binary (it's passed as base64 strings into the vault api).
There are 2 base flavors for each service:

• single element. e.g. encrypt(String, byte[], Optional<VaultTransitContext>)
• batch. e.g. encrypt(String, List<VaultTransitEncryptBatchItem>)

I know that the former can be achieved with the later, but when you have a single element, the weight of doing that with the batch api feels impractical (more instructions, no exception).
As far as the last method encryptString(String, String) it felt users could use a shortcut for the most simple situations where all you need is to encrypt a single string with no context.

• It would be good to support the injection of the already decrypted or encrypted (etc) values, but can we do it without Support the configuration property indirection with custom ConfigSource #3937 ?

hmm. I have not thought about this.
what is your use case exactly?
I think the primary use case for encrypt/decrypt is to store sensitive data in a primary store, typically a db. so I am not sure how injection could play a role. I do not see injection playing a role there.

there is actually another very good use case: a jpa converter to store entity attributes as encrypted strings.

[sberyozkin]

Hi @vsevel ,

Re the nonce,

I was actually hesitant to add support for it, as my understanding is that if you generated your keys after 0.6.1, then you should not use it.
if we leave it, may be we should not try to outsmart it, and let the users specify it themselves.

IMHO we should do the opposite, do not offer it at the API level, because as I said it is not something we should be expecting the users do correctly, the nonce generation has to be secure in its own way and since Quarkus offers the Vault integration, let Quarkus do the tricky things. Indeed, lets not do it at all :-), we can document that this engine targets the latest 0.63 version with the explanation why, and if ever needed the engine can drive the nonce generation, it is not really a user-level concern IMHO, even though it is probably a small utility only.

You are right, the engine should not be trying to be too smart and start deciding whether to generate the nonce based on some heuristics :-) but if the users will need it then they will just ask nicely with a property and the engine will help.

context[]

I was asking about the purpose of VaultTransitContext.context[] property

encrypt(...) duplications....

You are right, the shorter version can be simpler and indeed, having encryptString is a good idea,
I wanted to ask about byte[] - what else do you expect, apart from String be encrypted ? InputStream or byte[] available as JAX-RS parameters for example ?

I think the primary use case for encrypt/decrypt is to store sensitive data in a primary store, typically a db. so I am not sure how injection could play a role. I do not see injection playing a role there.

One would inject the already encrypted value if it needs to be available elsewhere, and decrypted value - for the same reason. For example, the API has a decrypt method - we don't know how the users will use it, they may try to use it to decrypt the incoming encrypted value or return a decrypted value over HTTPS for example.

there is actually another very good use case: a jpa converter to store entity attributes as encrypted strings.

Sounds interesting :-). May be lets tune the 'core' interface and then discuss how to support it. May be we should have a concept of TypeConverter which would turn JPA/etc entities into String and back...and have encrypt/etc methods accepting Objects, etc

[vsevel]

@sberyozkin

Indeed, lets not do it at all :-),

I agree but not for the same reasons ;-)

we can document that this engine targets the latest 0.63 version

actually 0.63 is way old (dec 2016). last version is 1.2.3 (sept 2019).

but if the users will need it then they will just ask nicely with a property and the engine will help.

if just one user is asking for it, then I would rather get him to provide the value, rather than implementing the algorithm to do it correctly, and having to maintain this level of complexity in quarkus. people that are using nonce, have created their keys back in 2016, or before. so they do not need us to do it, they just need to be able to pass the nonce created with their algorithm.
but like we said, let's not do it, and wait for feedback.

I was asking about the purpose of VaultTransitContext.context[] property

it maps to the context attribute in the api.

it is used for key derivation. see my example in #4888 (comment)

what else do you expect, apart from String be encrypted ?

actually everything. I have seen situations where every sensitive data is encrypted in the db, including booleans, numbers, dates, blobs.

One would inject the already encrypted value if it needs to be available elsewhere, and decrypted value - for the same reason

I have got to think more about that.

May be we should have a concept of TypeConverter which would turn JPA/etc entities into String and back...and have encrypt/etc methods accepting Objects, etc

I have tried implementing an AttributeConverter, but the issue I found was that the attribute metadata was not available.

I would have liked to write something like this:

@Entity
class Person {
  @Column...
  @VaultEncryptionKey(name="myapp", context="person_name");
  @Convert(converter = EncryptionStringConverter.class)
  String name;

@Converter
public class EncryptionStringConverter implements
  AttributeConverter<String, String> {
    @Override
    public String convertToDatabaseColumn(String paintext) {
    ...
    @Override
    public String convertToEntityAttribute(String encrypted) {
    ...

but in the converter I do not get metadata on the name attribute, so I can't read its annotations.
I thought about it 5 minutes. so there might be an elegant solution I did not see immediately.
I am thinking we should put that aside for now anyway and focus on the core api.

[vsevel]

@sberyozkin

One would inject the already encrypted value if it needs to be available elsewhere, and decrypted value - for the same reason. For example, the API has a decrypt method - we don't know how the users will use it, they may try to use it to decrypt the incoming encrypted value or return a decrypted value over HTTPS for example.

for your https example, it looks more like a custom jackson serializer. but then again we do not get attribute metadata in the StdSerializer contract. all we could do is to create a serializer for a given type (eg EncryptedString) and encrypt all instances of that type with a single key and no derivation. and may be that is enough (but it would be a lot better to be able to express the context at at the attribute level).
this is the same issue than for the jpa converter.

[sberyozkin]

@vsevel thanks, I got it about the context, definitely can be useful. Can you please have the constructors added to let users pass plain Strings as contexts as well ?

Re nonce, yes, and I'd just say, lets support the latest and the most secure version of the transit engine. In general, IMHO, since we are offering an integration, we should try to hide as much as possible from the users, nonce is not really a user-level concern (sorry, I'm repeating myself :-)), it is not a user problem that some of the older transit engines can be broken without nonces :-)

+1 to tackling the encryption of the more complex objects in the next phase.

[michael-schnell]

Just to give you an idea on how I plan to use the new API:

The General Data Protection Regulation (EU) requires that a user may request to delete it's personal data from a system. This is difficult in case of event sourced systems where the data is only stored in form of immutable events. The only way to achieve this, is to encrypt the sensitive parts of the message with a key that is uniquely used for that user. If the user requests to delete the data, only the private key is thrown away so no one can decrypt the data any longer.

This key will be stored in Vault and the encrypt/decrypt functions will be called to encrypt/decrypt the sensitive data.

Here is an example of such event:

{
   "event-id":"6fddc2e1-b64a-4df0-bb10-a2266a896c61",
   "event-timestamp":"2019-04-25T08:17:43.476+02:00[Europe/Berlin]",
   "entity-id-path":"PATIENT 386cec11-afdd-417b-b453-a140634e88bd",
   "personal-data": {
       "key-id":"PATIENT 386cec11-afdd-417b-b453-a140634e88bd",
       "key-version":1,
       "iv-version":2,
       "data-type":"PatientPersonalData",
       "content-type":"application/json; encoding=UTF-8; version=1",
       "encrypted-data":"I03eadJjZ0S4lR/eSe65bxDtVRPXkethrnJLxYTPLUKvnRQshq6njrnFQIZMSDWBgQHJlCRh7UMBHejxuxfSq1dXWIo6bnHYOa6zMJITKDgn2eBUfIx8KrAQULQrH7BAL4DEGI3oPDVcs6Y9o1pdfok8hY1cfRLN5FuSHqoXNTb4uc9gdoHT/T0Lv+Rd6kRbb6Wmeuro/j2NoDG2EKKTk7R5BgGXBxt3ob6XLdW6T0xqZ8hvCAIMEH4ESlYe/QVOc/JcOyzA4CnKNzxxrGHRMuDIYww+2dfG/ZqXIWFbuh+x/6zFF21h/cQ="
    },
    "year-born":"1939"
}

The "personal-data" is of type (EncryptedData.java) that has everything to decrypt and decode the data.

The interfaces "EncryptedData.Encrypter" and "EncryptedData.Decrypter" will be implemented using the new Quarkus Vault Transit Secret Engine. I'm currently preparing a Quarkus example for my DDD / CQRS libraries implementing also the Vault Secret Engine feature: ddd-cqrs-4-java-example

[vsevel]

@michael-schnell thanks for your feedback. indeed I made some progress already.
I had the initial low level vault client part done for a few operations (encrypt, decrpyt, sign and verify). they require some work because they all are batch oriented. and they look very close to each other, but all differ in subtle ways. I started also to look at the vault impl and that gave some ideas about some simplifications I could do.
there is more work to be done also on the config part specifically around deciding what properties should be passed as arguments, and what should stay as config properties (e.g. key version), and whether or not to distinguish encryption keys and sign keys, which do not support the same properties, or just make it just an encryption key with a combination of all possible properties and test at runtime based on the use case (my preference right now).
lastly I have been struggling with the api, trying a few things back and forth. for instance whether or not to use simple types (e.g. String for signatures, or ciphertext) or abstract types (e.g. class Signature) like in spring vault (my preference at this point for signatures and hmac).
I have seen also that the spring api returns the context as part of the Ciphertext and Plaintext objects, which I am not sure I agree with. I recognize that to be a design choice, so not entirely wrong, but not my preference right now.
anyway a lots of stuff going on. making progress, time permitting.
obviously your help is most welcomed, anyway you can: implementation, api design, test, doc, ...
stay tuned.

cc @sberyozkin

[sberyozkin]

@michael-schnell Your input is indeed very valuable, thank you :-)

[sberyozkin]

@vsevel The API is already looking good, thank you. We will complement with the more complex object support in the next phase and it will be super :-) Enjoy the weekend.

[vsevel]

I have made a thorough review on the key_version attribute. It is being used for encrypt, rewrap, sign and hmac.
this one is tricky because in the 2 first cases it is part of batch input: see here and here.
and in the 2 later cases, it is part of the body at the first level: here and here.
what is strange is that spring seems to get it correct for hmac but not for sign.

so for encrypt and rewrap I added the field in the batch object, but for the sign operation (and the hmac operation when we have it) I had to add a keyVersion attribute, which changed the method signature:
List<SigningResult> sign(String keyName, Integer keyVersion, List<SigningRequest> requests).
To keep the method stable, one option is to wrap keyVersion in a request class.
Leaving as is for now.

[sberyozkin]

@vsevel I'd not put the key version directly into the sign signature and do it the same as for the encrypt, the fact the Vault expects it in the different locations for different operations is not a problem for our API :-). Dealing with the various versions of the key is an advanced scenario, so if the users will need it to sign then they will set it as part SigningRequest and our implementation can ensure it is presented in the way understood by Vault

[vsevel]

so if the users will need it to sign then they will set it as part SigningRequest

You can specify the key version only once for the full batch (it is not part of the batch item). I would not know what to do with a version specified in each SigningRequest object. Split the batch in multiple sub-batches grouped by key version? Or assert that all SigningRequest objects have a consistent key version? That feels awkward don't you think?

Or introduce a wrapper object?

public class SigningBatchRequest {   
    Integer keyVersion;
    List<SigningRequest> requests;   
}

public interface VaultTransitSecretEngine {
...
List<SigningResult> sign(String keyName, SigningBatchRequest batchRequest);

I thought about that, but then it lost the consistency with the other services encrypt, decrypt, ...
If we really want to get rid of the keyVersion in the api, then I would lean toward introducing that kind of wrapper object. I am making the change to see how it looks like.

What do you think about the last changes @sberyozkin ?

[vsevel]

@sberyozkin all comments addressed or tentatively answered.

[sberyozkin]

@vsevel thanks, it is nearly there IMHO and is looking neat. I'd just like to finalize the discussion about how the errors are addressed

[sberyozkin]

@vsevel re the key version, for signing we have a single key version for all the requests in the batch, which is fine. But for encryption - it is per request specific in the batch. We should have the same mechanism at the API level IMHO. The fact that Vault expects it in the different places in the request for encrypt vs sign is not important IMHO

[vsevel]

But for encryption - it is per request specific in the batch

@sberyozkin are you suggesting to not support key version per request, but instead at the whole batch level?

[vsevel]

@sberyozkin by the way, I changed ClearData to BinaryData because that same object was being used in sign(String keyName, BinaryData input, TransitContext transitContext) where it is being called input in vault's api. Calling it clear data here did not make sense.

[sberyozkin]

@vsevel BinaryData seems OK, yes. But we can also ClearData for encrypts, and SignatureInput for signs ?

[vsevel]

@sberyozkin

sorry for a delay, I was traveling

no problem. I hope you had a good trip.

The only one which IMHO we need to do something about is the marshalling algorithm (base64url)

yes. this has been handled.

lets try to make it into 1.1.0, may be a bit tight

when is due 1.1? I though it was 6 months after 1.0...

by the way, I had to deal with testing a validate claims function in a separate PR, and found it useful for it to return a boolean :-). Having void and an exception for the validateSignature is good, though having a single boolean return for all the validateSignature might be OK too simply to support the flows like if (validateSignature(...))

you perfectly summarized my state of mind ;)
it really comes down to whether a signature that does not match a document is an error and is exceptional (then it should be an exception) or just an information such as 'this signature does not match that document ' (then it should be a return type). if this is the latter, having to deal with a void method makes the client code bulky because of the exception handling.

but I'm totally OK if you don't change the current signature for these methods, perhaps we should not change the interface now.

if you feel strong about changing it back, I do not have a problem. that should not be too long.
if not, I will leave it as is.
we can always ask guillaume about it. I think he has got a good sense of the user's perspective.

I have just pushed the last changes. waiting for the CI to pass.
if you do not have other comments at this point, we can open it up to guillaume.
thanks for your comments

[vsevel]

hello @michael-schnell
if you are still following that thread, could you look at the interface and comment on whether or not this is looking good for a first version? thanks

[sberyozkin]

@vsevel thanks; yes, it all looks good to me now. 1.1.0 is due shortly AFAIK,
@gsmet, Hi Guillaume, IMHO it is good, fits into the the already established Quarkus Vault ecosystem, not sure if we can get it into 1.1.0, have a look please if you can get some time, I believe @michael-schnell in interested to give this interface a try. thanks

[sberyozkin]

Please don't merge until @gsmet has had a look

[michael-schnell]

As the feature was now removed from 1.1.0 - Will there a snapshot JAR be available instead? (Like on https://oss.sonatype.org/content/repositories/snapshots/ ) - Would be good to allow already using it via an official Maven repo.

[vsevel]

hello all,

@michael-schnell I suppose you can clone my repo and build it locally. the only build I know of is from master.

@gsmet @sberyozkin I know it is a busy time of the year, but can we start considering it for 1.2? what is the next step? who could review the PR?

thanks

[sberyozkin]

@vsevel as far as I'm concerned it is ready to go in but indeed it is a very busy time for @gsmet :-). I added a 1.2.0 label.

[michael-schnell]

@vsevel as far as I'm concerned it is ready to go in but indeed it is a very busy time for @gsmet :-). I added a 1.2.0 label.

Any news here? Would be nice to have this merged to use it from snapshot for tests.

[gsmet]

@vsevel I added a couple of small comments on the doc. Otherwise, it just needs a rebase and we can merge it for the next version.

Thanks!

[lordofthejars]

Shouldn't the adoc about Vault be updated as well with the feature added here? Or as a new adoc, but I think it should be documented.

[vsevel]

here is an update on the PR based on the latest comments:

@gastaldi the PR was rebased. I will do a final squash before the merge.

@lordofthejars suggested to update the existing guide, or create a new one. Initially I thought the javadoc would be enough, at least for specialists. but I recognize that some additional documentation would help a lot with adoption. there is a lot of material already in the existing guide, which focuses on the kv secret engine and vault config source. for the sake of simplicity I had already decided to leave apart other topics that I thought were worth documenting (k8s authentication and dynamic db credentials). so I would rather create a separate guide for the transit engine. but I am open for suggestions. do we want to have just one guide per extension, or one guide per topic?

@gsmet I applied all your recommendations with respect to documentation. I did a full review and found also some other cases that were worth correcting also: 7ae12d5

however, I was not able to get the JacksonModuleBuildItem to work as explained in #5147 (comment)

out of curiosity, is there any javadoc documentation for apis (e.g. VaultTransitSecretEngine or VaultKVSecretEngine), or is this just for config?

If I start working on the guide, when should that be ready?

[lordofthejars]

@vsevel as we add more and more support to Vault we are going to have the problem you mention here with documentation. So I think that creates a new document might be the best approach and when we have the full support of Vault, then we could rearrange all the Vault part.

[sberyozkin]

@vsevel Can you please do some basic initial doc about the transit engine so that we can get this PR for 1.2.0 ? I guess by the end of this week would be ideal ? thanks

[sberyozkin]

Hi Vincent @vsevel How is it going, we have just a couple of days before 1.2.0 CR1.
Hi Alex @lordofthejars, perhaps we can have a doc issue opened and assigned to Vincent, I think Guiiluame was saying earlier the docs can be added even after CR1/etc (which we did for the earlier versions), what do you think ?

[gsmet]

I squashed everything and rebased. If everything is green tomorrow morning, I'll merge it.

[sberyozkin]

@gsmet thanks :-)

[vsevel]

@sberyozkin @gsmet thanks for merging. last week I was too busy in the evening, and could not find during the weekend. I saw you created #6636. you can assign it to me. I will create a first draft this week.