Improve authorization checks for CN app APIs #2509
Conversation
rautenrieth-da
force-pushed
the
rautenrieth-da/api-auth
branch
6 times, most recently
from
f188239 to
9623c67
Compare
Signed-off-by: Robert Autenrieth <[email protected]>
Signed-off-by: Robert Autenrieth <[email protected]>
[ci] Signed-off-by: Robert Autenrieth <[email protected]>
[ci] Signed-off-by: Robert Autenrieth <[email protected]>
[ci] Signed-off-by: Robert Autenrieth <[email protected]>
[ci] Signed-off-by: Robert Autenrieth <[email protected]>
[ci] Signed-off-by: Robert Autenrieth <[email protected]>
[ci] Signed-off-by: Robert Autenrieth <[email protected]>
[ci] Signed-off-by: Robert Autenrieth <[email protected]>
rautenrieth-da
force-pushed
the
rautenrieth-da/api-auth
branch
from
9623c67 to
47a27ad
Compare
rautenrieth-da
changed the title
|
/cluster_test |
|
Deploy cluster test triggered for Commit 47a27adcdec237f249b70c1a440b95e56186ccb1 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/35811 |
|
/hdm_test |
|
Deploy HDM pipeline triggered for Commit 47a27adcdec237f249b70c1a440b95e56186ccb1 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/35835 |
| export VALIDATOR_AUTH_AUDIENCE | ||
|
|
||
| token=$("${VALIDATOR_DIR}/get-token.py" administrator) | ||
| token=$("${VALIDATOR_DIR}/get-token.py" ledger-api-user) |
There was a problem hiding this comment.
administrator is the wallet user, ledger-api-user is the app operator user.
| get: | ||
| tags: [wallet] | ||
| x-jvm-package: wallet | ||
| x-jvm-package: status.wallet |
There was a problem hiding this comment.
package wallet: requires actAs for the wallet party
package status.wallet: requires just an authenticated user, no rights checked
| get: | ||
| tags: [sv] | ||
| x-jvm-package: sv_admin | ||
| x-jvm-package: sv_operator |
There was a problem hiding this comment.
package sv_public: au auth whatsoever
package sv_operator: requires actAs on the sv operator party
package sv_admin: requires actAs on the sv operator party and participant admin
| consoleEnvironment.run { | ||
| httpCommand( | ||
| HttpSvAppClient.OnboardValidator(validator, secret, BuildInfo.compiledVersion, contactPoint) | ||
| HttpSvPublicAppClient.OnboardValidator( |
There was a problem hiding this comment.
Maybe even call it HttpSvNoAuthAppClient (don't know which one is scarier)
| def cometBftNodeStatus(): definitions.CometBftNodeStatusResponse = | ||
| consoleEnvironment.run { | ||
| httpCommand(HttpSvAppClient.GetCometBftNodeStatus()) | ||
| httpCommand(HttpSvPublicAppClient.GetCometBftNodeStatus()) |
There was a problem hiding this comment.
why is the node status public? (just curious)
| ): definitions.CometBftJsonRpcResponse = | ||
| consoleEnvironment.run { | ||
| httpCommand(HttpSvAppClient.CometBftJsonRpcRequest(id, method, params)) | ||
| httpCommand(HttpSvPublicAppClient.CometBftJsonRpcRequest(id, method, params)) |
There was a problem hiding this comment.
This also seems strange to be public?
| ) | ||
| } | ||
|
|
||
| @Help.Summary("Prepare a validator onboarding and return an onboarding secret (via admin API)") |
There was a problem hiding this comment.
is the comment still correct? (via admin API)
| isDeactivated = false, | ||
| ) | ||
| .id | ||
| assertRejectedAsUnauthorized(userId) |
There was a problem hiding this comment.
nice cleanup thanks
| * ==Authorization== | ||
| * | ||
| * - user must exist on the participant and be active | ||
| * - primary party must be set for the user and equal to the app operator party |
There was a problem hiding this comment.
you say app operator party, but it is actually the party parameter on this class right? slightly confusing cause you mix where this is used and what is defined here I think, rather refer to party with backticks
There was a problem hiding this comment.
(and use @param in the doc if needed)
| * ==Authorization== | ||
| * | ||
| * - user must exist on the participant and be active | ||
| * - primary party must be set for the user and equal to the app operator party |
There was a problem hiding this comment.
| * - primary party must be set for the user and equal to the app operator party | |
| * - primary party must be set for the user and equal to the `adminParty` |
| * - user must exist on the participant and be active | ||
| * - primary party must be set for the user and equal to the app operator party | ||
| * - user must have actAs rights for the app operator party | ||
| * - user must have ParticipantAdmin rights |
There was a problem hiding this comment.
| * - user must have ParticipantAdmin rights | |
| * - user must have ParticipantAdmin rights | |
| @param adminParty the app operator party |
| authenticateLedgerApiUser(operationId) | ||
| .flatMap { authenticatedUser => | ||
| onComplete( | ||
| rightsProvider.getUser(authenticatedUser) zip rightsProvider.listUserRights( |
There was a problem hiding this comment.
if you always do this zip, you might as well change the rightsProvider and have a getUserAndRights or something like that
| import scala.concurrent.{ExecutionContext, Future} | ||
| import scala.util.{Failure, Success} | ||
|
|
||
| class UncachedUserRightsProvider( |
There was a problem hiding this comment.
nitpick but I would just call this something like LedgerUserRightsProvider
| get: | ||
| tags: [sv] | ||
| x-jvm-package: sv | ||
| x-jvm-package: sv_public |
There was a problem hiding this comment.
So not sure about this one, why that's needed to be fully public
There was a problem hiding this comment.
This endpoint didn't have any auth before, here we're just renaming things to make it more clear.
Ideally this would have some kind of auth, but CometBFT will be replaced by a different consensus layer in the near future and we wanted to limit the number of breaking changes, so I left it as is.
| post: | ||
| tags: [sv] | ||
| x-jvm-package: sv | ||
| x-jvm-package: sv_public |
There was a problem hiding this comment.
So not sure about this one, why that's needed to be fully public
| "$ref": "#/components/schemas/OnboardSvSequencerResponse" | ||
| "400": | ||
| $ref: "../../../../common/src/main/openapi/common-external.yaml#/components/responses/400" | ||
| /v0/devnet/onboard/validator/prepare: |
There was a problem hiding this comment.
Can we prevent this API and other devnet-only ones to exist on non-devnet? or would we need to do many openapi spec files or something like that if we really wanted to? (or do we do, or can we do, a proxy thing that 'removes this' and returns notfound or something on non-devnet)
There was a problem hiding this comment.
The implementation returns a 501 Not Implemented on any non-devnet network where the isDevNet flag is false. Do you want to remove the endpoint because we don't trust this check, or because we want to avoid parsing the request in the first place?
There was a problem hiding this comment.
avoid handling it, but I guess it's not really important, I just thought it would be nicer if it responded like any other API that is not there
| * passing their onboarding secret in the request payload. | ||
| * | ||
| * Protection: The onboarding secret is unguessable and single-use. | ||
| * Note that we are still spending some resources on verifying the secret. |
There was a problem hiding this comment.
So this is an easy target for an attack I guess.. but can prevent issues with rate limit etc.
| respond: v0.SvResource.GetSvOnboardingStatusResponse.type | ||
| /** Intended use: Used by other SV operators | ||
| * | ||
| * Protection: Endpoint is protected by IP allowlisting |
There was a problem hiding this comment.
This comment will get out of date..
There was a problem hiding this comment.
why is this not sv operator?
There was a problem hiding this comment.
Intended use: Used by other SV operators
This is called by other SVs to onboard their node. Other SVs do not have any token for your sv app, so this can't have auth. We have to rely on IP whitelisting instead (like we do for many other requests between SV nodes).
| implicit val tc = extracted | ||
| /** Intended use: The SV app UI. | ||
| * | ||
| * Protection: None for backwards compatibility reasons |
There was a problem hiding this comment.
Maybe create an issue to deprecate and move
There was a problem hiding this comment.
There's one on the line below 🙂
| respond: v0.SvResource.GetCometBftNodeStatusResponse.type | ||
| /** Intended use: Interacting with CometBFT node monitoring/debugging/testing purposes | ||
| * | ||
| * Protection: Endpoint is protected by IP allowlisting |
There was a problem hiding this comment.
Can't this move to sv_operator as well? (or plan to make a move)
There was a problem hiding this comment.
Same as above: cometbft will be replaced, changing auth here is not worth the effort.
| respond: v0.SvResource.OnboardSvSequencerResponse.type | ||
| /** Intended use: Used by other SV operators | ||
| * | ||
| * Protection: Endpoint is protected by IP allowlisting |
There was a problem hiding this comment.
I think I would remove these comments
There was a problem hiding this comment.
(maybe change it to a TODO to move it out of public)
There was a problem hiding this comment.
Same as above, endpoints that are called by other svs can't have auth.
| walletManager, | ||
| userRightsProvider, | ||
| loggerFactory, | ||
| "splice wallet realm", |
There was a problem hiding this comment.
nitpick extract to constant / val
| mat: Materializer, | ||
| tracer: Tracer, | ||
| ) extends v0.ScanproxyHandler[TracedUser] | ||
| ) extends v0.ScanproxyHandler[AuthenticatedRequest] |
| mat: Materializer, | ||
| tracer: Tracer, | ||
| ) extends v0.ValidatorAdminHandler[TracedUser] | ||
| ) extends v0.ValidatorAdminHandler[AdminUserRequest] |
| .flatMap { authenticatedUser => | ||
| onComplete( | ||
| // Double zip is a bit ugly... | ||
| rightsProvider.getUser(authenticatedUser) zip rightsProvider.listUserRights( |
There was a problem hiding this comment.
if you change rightsProvide to have one method for this there is one less zip :-)
|
|
||
| /** @param user The authenticated user name. | ||
| * @param userWallet The wallet associated with the authenticated user. | ||
| * We need to look up the wallet in the extractor to get the end user party, |
There was a problem hiding this comment.
nitpick: here you are describing what code somewhere else is doing, I would prefer to document that differently, this should just describe the field
| Revoking user rights on the participant will revoke access to the corresponding API endpoints. | ||
|
|
||
| In general, endpoints that required authentication before will now check that the authenticated user | ||
| is not deactivated on the participant has has ``actAs`` rights for the relevant party |
There was a problem hiding this comment.
| is not deactivated on the participant has has ``actAs`` rights for the relevant party | |
| is not deactivated on the participant has ``actAs`` rights for the relevant party |
| rights may use the SV or wallet UIs, but may not perform administrative actions like | ||
| hard synchronizer migrations. | ||
|
|
||
| - ``/v0/admin/domain/pause`` |
There was a problem hiding this comment.
I think this list should follow directly after
The following SV app endpoints now require the user to have participant admin rights in
the participant user management service.
And after the list the next paragraph
| - ``/v0/admin/domain/data-snapshot`` | ||
|
|
||
| Note that only the service users of the SV and validator apps should automatically have participant admin rights. | ||
| If you are using other users to access the above endpoints, check their rights. |
There was a problem hiding this comment.
| If you are using other users to access the above endpoints, check their rights. | |
| If you are using other users to access the above endpoints, please check their rights. |
|
|
||
| - SV app ``/v0/dso`` is currently public, but will require authorization as SV operator, | ||
| similar to most other SV app endpoints. | ||
| Use the corresponding public endpoint in the scan app if you need to fetch DSO info. |
There was a problem hiding this comment.
Maybe just state the actual endpoint in scan
| Use the corresponding public endpoint in the scan app if you need to fetch DSO info. | |
| Use the public ``/v0/dso`` endpoint in the scan app if you need to fetch DSO info. |
|
@rautenrieth-da this change seems somewhat risky in terms of breaking some of the workflows SVs may be used to and non-urgent. Any reason not to postpone until after the 0.5 migration? |
The PR will probably quickly conflict with changes on main because it moved around many methods in the I agree that it could break some workflows, hence the detailed changelog entry. I'll leave it open and try to keep it up to date with main until 0.5.0 then. |
3 participants
Fixes https://github.com/DACH-NY/canton-network-internal/issues/1639
Summary of changes:
UserRightsProvideris a new trait for fetching the rights for a userAuthExtractorhas been refactored, now has several subclasses, each implementing on auth requirement:AuthenticationOnlyExtractor: requires authenticated user, but no other special authorization. Useful for endpoints returning "public" data where we don't want to allow anonymous access for audit or rate limiting purposes.UserWalletAuthExtractor: requires authenticated user, authorized toactAsthe party associated with the user through aWalletAppInstallcontract. Useful for endpoints that interact with some wallet.AdminAuthExtractor: requires authenticated user, authorized toactAsthe app operator party AND have participant admin rightsActAsKnownPartyAuthExtractor: requires authenticated user, authorized toactAsa given party (usually the app service party)Http*Handler: all endpoints in the same handler require the same auth. Auth is enforced by using one of the above auth extractors when constructing the routes, and the auth extractors returning unrelated types. As a result, some endpoints have moved:HttpWalletHandlerhave moved toHttpStatusWalletHandlerbecause they don't require the user to have a wallet installed.HttpSvPublicHandler(no auth),HttpSvOperatorHandler(actAs operator party),HttpSvAdminHandler(actAs operator party AND participant admin)