Repository navigation
Conversation
Signed-off-by: Ian Chin Wang <ian.chin.wang@oracle.com>
Signed-off-by: Ian Chin Wang <ian.chin.wang@oracle.com>
| ) | ||
|
|
||
| router.Handle(keyIdExpr+"/transfer", transferKeyHandler).Methods(http.MethodPost) | ||
| router.Handle(keyIdExpr+"/transfer", authMiddleware(transferKeyHandler, authz)).Methods(http.MethodPost) |
There was a problem hiding this comment.
please revert this change, this is by design, no auth needed for attestation backed key transfer.
There was a problem hiding this comment.
Hi @arvind5, is there a reason why key:transfer permission was explicitly required for the old /POST/transfer API but not the latest one? Should KBS allow key transfer request from any user who knows the key ID, even though the admin tries to excludes the users from doing a key transfer?
There was a problem hiding this comment.
we never had auth for POST /transfer API, it was always attestation backed transfer. keys:transfer permission was specifically added for POST /keys/{keyid} API which releases key without attestation, once authenticated.
There was a problem hiding this comment.
Hi @arvind5, I believe in the user perspective, multiple assumptions has to made for keys:transfer and POST /keys/{keyid}. The user has to derive the conclusion using POST keys:transfer only for /keys/{keyid} API which releases key without attestation but not for POST /transfer API from the README. The use of the term transfer seems to be ambiguous here.
In the production case, it's preferred to limit who can access the key even further to only those who are supposed to. If there are 2 workloads running on the same environment (host) belonging to different users with the expectation that one should have access while the other should not, it's not possible with the current implementation, not to mention KBS may releasing key even without attestation. I'm not sure if this project was created just as a prototype, but ideally, the per-key access permission is substantial for this project and we hope this can close the gap between the practical usage of KBS and the expectation of Intel for this repo.
There was a problem hiding this comment.
Hi @cowbon, the intuition behind exposing POST /keys/{keyid} API was to enable KBS admin users to get the key data for encryption purposes, whereas POST /keys/{keyid}/transfer API is meant to be used by workloads who want to get hold of the key for decryption purposes. The workloads could be owned by different users who may not be having the admin privileges, they will be provided with the right key-id for provisioning into their workloads. I agree that it could be confusing and maybe we can rename the permissions differently, or add more clarity to the README.
Having said that, I absolutely agree with your claim around 2 workloads running on same environment, and I think this could be solved via key transfer policies. Since the attestation and policy evaluation are the key criteria for key release via POST /keys/{keyid}/transfer API, I believe distinguishing 2 workloads based on their attributes would be more appropriate compared to provisioning them with different set of credentials. The current key transfer policy schema may not be supporting it, but we can enhance the schema to address this issue. Let me know your thoughts.
There was a problem hiding this comment.
Thanks for the reply. I believe I have more understandings on the design philosophy now. Regarding the naming of the permission for admin users to get the key data for encryption purposes, I'd recommend to use keys:info or keys:getinfo for this purpose. Then we should mention what permissions are available and their effects in README. I'm happy to work on this once we reach the consensus.
Regarding the use of key transfer policy to identify which workload can access the key, rather than the login credentials, seems like a long term goal for us. The challenge would be dealing with the workload identify in KBS. We may recourse to SPIFFE and add a workload_identity attribute to policies, IAM policies from the cloud vendors, or if you have any other suggestion than filtering out who can access the key without using a whitelist of KBS users in the policy, feel free to share it to facilitate the collaboration.
| policyIDPath := apiPathPrefix + "/key-transfer-policies/" + constant.UUIDReg | ||
| userIDPath := apiPathPrefix + "/users/" + constant.UUIDReg | ||
|
|
||
| opt := token.SetScopes(token.NewScope(constant.KeyTransferPolicyCreate, apiPathPrefix+"/key-transfer-policies$", "^POST$"), |
There was a problem hiding this comment.
do we need ^ and $ in http method as well or just restricting path regex would suffice?
API docs and user-input validation use key-transfer-policies (hyphens), while the actual enforced scope constants use key_transfer_policies (underscores).
keys:transferis active forPOST /keys/{id}, but does not protect the documented/current evidence-transfer endpointPOST /keys/{id}/transfer.key-transfer-policies:createwith this PR becomeskey_transfer_policies:createbefore validation and persistence, matching the JWT authorization scope.Regarding
keys:transfer,