feat: add a new management api for secrets

[sbeuzit06]

What this PR changes/adds

This PR adds a new management API that enables to manage secrets from the vault.

Why it does that

In case of the connector is hosted on behalf of another entity, this entity might not have access to the underlying vault. In that case, this API can be used to add or remove secrets from the vault.

Linked Issue(s)

Closes #4090

[github-actions]

We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.

[paullatzelsperger]

A few general remarks:

• I think it should be called SecretsApi etc., i.e. using the plural, otherwise people could be mislead into thinking it's some sort of secret API
• Please do not introduce domain relations (Secrets -> Assets) just out-of-hand. This is a more complicated topic, and one that must be discussed and designed first. If this is a requirement, then I suggest opening a scoped discussion first.
• I would generally avoid transmitting stuff in the request URL. secret aliases are not secrets per se, but I wouldn't take any chances.
• Please add an End-To-End Test. Feel free to take a look at the AssetApiEndToEndTest or the CatalogApiEndToEndTest etc. for an example.

Other comments inline.

[ndr-brt]

please add some end-to-end tests (as done in system-tests/management-api for the other endpoints

[rafaelmag110]

I have one concern regarding a possible exploit this API opens.
As is, an ill intentioned actor can use this to mess with secrets that have been registered by the operator of the connector, and are necessary for its proper functioning (public/private certificates, idp releated secrets, ...).

Two proposals that come to mind are:

• Adding a config that enables an operator of the connector to config the API so a prefix can be added to the secrets created by the it.
• Adding the possibility to change the underlying vault directory where the secrets created by the API will be stored

[paullatzelsperger]

I have one concern regarding a possible exploit this API opens. As is, an ill intentioned actor can use this to mess with secrets that have been registered by the operator of the connector, and are necessary for its proper functioning (public/private certificates, idp releated secrets, ...).

That is true for any other Management API as well. Malicious actors could easily nuke their connector deployment if they delete stuff they shouldn't. Which is all the more reason to properly secure the Management API.

• Adding a config that enables an operator of the connector to config the API so a prefix can be added to the secrets created by the it.

not sure how that would help the supposed exploit

• Adding the possibility to change the underlying vault directory where the secrets created by the API will be stored

Some powerful vault impls (Hashicorp Vault) support that, but some vaults, such as Azure KeyVault are more geared towards role-based-access-control, so there is no way that I know of to realize user-based access control on "subfolders".

[rafaelmag110]

That is true for any other Management API as well. Malicious actors could easily nuke their connector deployment if they delete stuff they shouldn't. Which is all the more reason to properly secure the Management API.

Isn't it a bit different? If someone who has access to the DMA mistakenly deletes a resource (i.e. asset or policy) its up to them to restore it. But if they are able to, say, delete the private certificate the connector uses to encrypt EDRs is a concern for the operator of said connector. At least from my point of view...

not sure how that would help the supposed exploit

If the SecretsApi always prefixes its keys, it is unlikely that a malicious actor can access non prefixed keys, only if the operator made the mistake to prefix them in the same way.

[paullatzelsperger]

Isn't it a bit different? If someone who has access to the DMA mistakenly deletes a resource (i.e. asset or policy) its up to them to restore it. But if they are able to, say, delete the private certificate the connector uses to encrypt EDRs is a concern for the operator of said connector. At least from my point of view...

I don't think so, because if you give users access to a certain class of information, be they assets, policies or secrets, you're handing over full control. If an operational scenario requires that users have only read access to secrets, but write access should be blocked, that could be solved in the vault directly. Most vaults have some sort of RBAC.

If the SecretsApi always prefixes its keys, it is unlikely that a malicious actor can access non prefixed keys, only if the operator made the mistake to prefix them in the same way.

I may not be understanding this fully, but it sounds like a very brittle and insecure solution, that could easily be circumvented by guessing the secret alias, or looking at EDC code.

[bscholtes1A]

A few general remarks:

• I think it should be called SecretsApi etc., i.e. using the plural, otherwise people could be mislead into thinking it's some sort of secret API
• Please do not introduce domain relations (Secrets -> Assets) just out-of-hand. This is a more complicated topic, and one that must be discussed and designed first. If this is a requirement, then I suggest opening a scoped discussion first.
• I would generally avoid transmitting stuff in the request URL. secret aliases are not secrets per se, but I wouldn't take any chances.
• Please add an End-To-End Test. Feel free to take a look at the AssetApiEndToEndTest or the CatalogApiEndToEndTest etc. for an example.

Other comments inline.

Agree with all remarks.
They have been all processed as per last commit.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 17.57576% with 136 lines in your changes are missing coverage. Please review.

Project coverage is 19.05%. Comparing base (7f20ba5) to head (5cb47c1).
Report is 243 commits behind head on main.

❗ Current head 5cb47c1 differs from pull request most recent head fa17a58. Consider uploading reports for the commit fa17a58 to get more accurate results

Files	Patch %	Lines
...ontrolplane/services/secret/SecretServiceImpl.java	0.00%	31 Missing ⚠️
...trolplane/services/secret/SecretEventListener.java	0.00%	25 Missing ⚠️
...rg/eclipse/edc/spi/types/domain/secret/Secret.java	0.00%	14 Missing ⚠️
...cret/validation/JsonObjectToSecretTransformer.java	0.00%	11 Missing ⚠️
...tor/api/management/secret/SecretsApiExtension.java	0.00%	9 Missing ⚠️
...se/edc/connector/secret/spi/event/SecretEvent.java	0.00%	9 Missing ⚠️
...ret/transform/JsonObjectFromSecretTransformer.java	0.00%	8 Missing ⚠️
...management/secret/validation/SecretsValidator.java	0.00%	5 Missing ⚠️
.../edc/connector/secret/spi/event/SecretCreated.java	0.00%	5 Missing ⚠️
.../edc/connector/secret/spi/event/SecretDeleted.java	0.00%	5 Missing ⚠️
... and 5 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #4138       +/-   ##
===========================================
- Coverage   71.74%   19.05%   -52.70%     
===========================================
  Files         919     1003       +84     
  Lines       18457    20444     +1987     
  Branches     1037     1149      +112     
===========================================
- Hits        13242     3895     -9347     
- Misses       4756    16436    +11680     
+ Partials      459      113      -346     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bscholtes1A]

Isn't it a bit different? If someone who has access to the DMA mistakenly deletes a resource (i.e. asset or policy) its up to them to restore it. But if they are able to, say, delete the private certificate the connector uses to encrypt EDRs is a concern for the operator of said connector. At least from my point of view...

I don't think so, because if you give users access to a certain class of information, be they assets, policies or secrets, you're handing over full control. If an operational scenario requires that users have only read access to secrets, but write access should be blocked, that could be solved in the vault directly. Most vaults have some sort of RBAC.

If the SecretsApi always prefixes its keys, it is unlikely that a malicious actor can access non prefixed keys, only if the operator made the mistake to prefix them in the same way.

I may not be understanding this fully, but it sounds like a very brittle and insecure solution, that could easily be circumvented by guessing the secret alias, or looking at EDC code.

On top of what @paullatzelsperger mentioned, it is also worth emphasizing that such API will typically be exposed over the internet through an API gateway (such as Azure APIM) which gives you full freedom to create inbound policies for the incoming traffic so that only secrets with alias matching a given pattern can be updated, removed...

[paullatzelsperger]

a few nits an niggles here and there

[bscholtes1A]

please add some end-to-end tests (as done in system-tests/management-api for the other endpoints

done

[ndr-brt]

there are still 6 unresolved comments on my previous review (and 1 from Paul's one).
please solve them

[bscholtes1A]

there are still 6 unresolved comments on my previous review (and 1 from Paul's one). please solve them

All comments should be processed now, could you please check again

[paullatzelsperger]

one wrong package, but good for me otherwise

[bscholtes1A]

one wrong package, but good for me otherwise

fixed

[ndr-brt]

LGTM