Provide public key encryption via transit engine

[Gabrielopesantos]

Issue: #7256
Description:
Added support to import public keys (RSA, ECDSA) in transit engine.
For that, an additional field public_key was added to the "Import Key" endpoint and if filled, will import a public key. If ciphertext is also filled, public_key is ignored and only the private key is imported.

On the "Import Key Version" endpoint, three new fields were added, public_key, public_key and bump_version. version has the same behavior as in "Import Key" endpoint. bump_version defines if a new key version is going to be imported or an existing version updated. Is set to "true" by default to stay aligned with the current behaviour so each operation creates a new key version instead of trying to update an existing version. If set to "false", the value of "version" is taken into account and is the key version to be updated.

"Encrypt" endpoint was also adapted to make use of the public key if the private key isn't in the KeyEntry.

I am not sure if this PR is finished as there might still be some transit endpoints that need to be slightly changed given the fact that can exist keys without Key or RSAKey filled which can lead to unexpected errors (I think). Some documentation might need to be updated and test cases that I haven't considered.

[victorr]

Hi @Gabrielopesantos, I will be reviewing your PR, thank you for submitting it.

In the meanwhile, can I suggest that you please update the relevant documentation? You can find the API documentation under website/content/api-docs/secret/transit.mdx. File website/README.md has useful information about writing documentation, but it should be a straightforward process to perform the updates.

Update: I have unassigned myself from the PS since others are already reviewing.

[stevendpclark]

Hi @Gabrielopesantos,

I've given this a first round of review, it's a good first pass at the feature! I've added a few comments as part of the review (sorry got a bit carried away at one stage). As pointed out by yourself and @victorr this will need some documentation updates.

More importantly at least in my very quick testing the following three APIs now panic due to nil pointer issues, also I think we need to take in the public key data in PEM format

These are the three APIs I found that panic at the moment.

• read transit/keys/test-rsa
• write transit/sign/test-rsa
• write transit/verify/test-rsa

Please let me know if anything is unclear, and thanks for the initial submission!

[stevendpclark]

Also updated the tests. While working on them I was trying to print the output response of the export request but for some reason the response Data field was empty, is that expected or was I doing something wrong?

The two things that come to mind would be

1. Make sure the key on import has the exportable field set to true, you'd have a Data field with an error key
2. Verify the key name for the export is correct, you'd see a nil, nil response back from this case.

Since the majority of transit tests aren't using the logical API client, but going directly into the HandleRequest call on the backend, checking for err technically isn't enough as we might also have an error within the response field, if the handler returned something like logical.ErrorResponse("an error"), nil.

[stevendpclark]

@Gabrielopesantos Looks great, I'd say this is in a good spot now code-wise, perhaps just a test or two around the export would be nice to have.

The only remaining thing I'd say would be to clean up the note thingy I posted about above, and resolve the TODO's in the docs

[cipherboy]

Wow @Gabrielopesantos! A lot of great work here! :D

I think this is good to merge, I might suggest we clean up a few more things before 1.14 GAs, but we've got some time and there's enough good stuff here that I think we should merge it now and we'll pickup docs and some of these nits here shortly. :-)

[cipherboy]

Not quite sure I agree with this change @stevendpclark btw. I think we should probably continue exporting only the private key and not automatically switch to the public key if the private key is not available, and instead err out.

[Gabrielopesantos]

Now that I think about it I am also not sure. I see at least three sensible ways of what the /export endpoint can return when a key version doesn't have a private key imported:

• Return an error
• Return the public key (currently implemented in the PR)
• Return an empty string

As the return public key and empty string change the behaviour of what is expected to be returned depending on the key to export, I might be prone to confusion and errors unless well documented.
With that, returning an error seems the most valid approach.

[cipherboy]

@Gabrielopesantos Yeah, @stevendpclark and I went looking a bit on Friday to try and understand this question better and we got to that it was just a quirk of how things were done. I forget exactly what we decided, but I think we thought leaving it private-key only was fine.

Steve, do you recall if we discussed adding a new public type export and maybe a both type export? Not sure what you thought of that?

Regardless, I think we're fine merging this as-is, everything else here are just minor nits :-)

[stevendpclark]

We landed on these items should be skipped from the output I believe.

It will technically "break" the existing API, but made the most sense if memory recalls as an empty string value would most likely break many assumptions, returning an error would break the form of export that exports all keys in one go and returning the public key doesn't really fit well with the export types.

[cipherboy]

I'm not quite sure I agree with this behavior @stevendpclark -- I'm not sure this should really be a parameter. Importing a public key always bumps version if it doesn't match the existing private key, importing a private key bumps the version if it doesn't match the public key, and re-importing something matching shouldn't bump the version either. I think there's enough context to do the right thing without adding this parameter IMO.

[Gabrielopesantos]

Unless I am not considering or forgetting something, with your suggestion, it seems easy for public keys to be "detached" from their private counterpart, whether on purpose, as other versions are imported on the same key, or by mistake. Just a thought, don't know if would matter or cause any problems.

[stevendpclark]

@cipherboy and I discussed this a bit and I believe we came to the conclusion we'd want the behavior he is mentioning but to only operate on the current latest version of the referenced key. That way we can get rid of both new options, the import versions operates more or less the same as it does today in that new imports create a new version except for this one use case in which you happen to match the existing public key and we swap it to a private key.

Again probably something we will readjust afterwards.

[Gabrielopesantos]

About merging the PR in its current state, there are still some things missing and others that could be improved but I am ok with that if you guys think it's best.
The PR is also a bigger than what is usually expected, at least in my opinion.

I would gladly work on what's missing once you decide what exactly you expect.

(Forgot the send this message the other day)

[cipherboy]

\o hey @Gabrielopesantos! Congrats on the merge and sorry about the delay :-) Wasn't sure if we'd get the time to get this in this next release or not.

[haumanto]

Hi,

I keep getting error when trying to impor this public key. Please help.

{
"type": "rsa-2048",
"public_key": "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAt1jJLdS4xnClx0Cy/JyaduQE6vGhy8cHz/t3DcZyqFSPG3BfSUAytSWJWj/XS0SdIIq1D/peO7FQnSF4oiARhmO0U9QXE79WKXzZEXaTNMPr7bVDtv7CpfMv8yjVgiVhc2Riv2/+zTtjmp05Yl0XAZT57O9Sbvqnc1UlHD/kwfTKEV1TAbdy81sfYf5w1SKUUQDhoTNmpTm/GdhqFczuoOBDKjjlLV/1G3niuAPlC06wAYaHKNpn2SDa7P0XNQgYTA6CuIFqH/gMfai0CVsTbUzNIKCB/p9/n+5G1vszV+hufasx3xfns7Mb3SEshLLsczt2IhefU8txUwW4Bhrf8QIDAQAB"
}