Authorization rule changes for IDPs isolation [DPP-1336] #15848

skisel-da · 2022-12-09T09:34:48Z

No description provided.

pbatko-da

General question:

Will it be possible to set up a user he admin right and in a non default idp?
What if the user management service is enabled and someone uses a custom daml access token (one doesn't mention the user id). Can such a request succeed?
To fetch themselves in GetUserRequest a user doesn't need to provide it's user id but it needs to provide its idp id - this seems a bit unintuitive. Maybe if we renamed the default idp from the empty string to a some other value (e.g. "default") then maybe w could make the identity_provider_id fields in the requests default to the current user's idp?

I think these changes need be accompanied by a suite authorization tests because the logic here is really non-trivial.

pbatko-da · 2022-12-21T11:51:17Z

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/Authorizer.scala

+      // Claim is valid only for the specific Identity Provider,
+      // and identity_provider_id in the request matches the one provided in the claim.
+      Left(AuthorizationError.InvalidIdentityProviderId(id))
+    case _ =>


When claims.identityProvider is the default idp and requestIdentityProviderId is None then you're succeeding, but requestIdentityProviderId being None actually means that request idp validation failed.

I think this method should accept only valid requestIdentityProviderIds i.e. requestIdentityProviderId: IdentityProviderId

If requestIdentityProviderId is not provided - it's NOT an authorization problem, it will be validated out in the layer below. It is an invalid argument which will be validated further. Not sure how to act here to make sure proper layers are responsible for errors.

My understanding of this code is that in line 129 Left(AuthorizationError.InvalidIdentityProviderId(id)) is returned also requestIdentityProviderId is None.

Also, could it even be the case that requestIdentityProviderId is not provided? Because if it's the empty it actually means the default idp

Let me first define here what is what.

claims.identityProviderId is what is provided from the token. So two options here are possible:

default IDP (token is issued by the auth config defined in the default configuration)

non-default IDP having concrete value (as it is of type IdentityProviderId)

requestIdentityProviderId is what we extracted out from the request. Here we distinguish the following cases:

None, means identity_provider_id from the request which cannot be recognized/parsed. For example String with more than 255 chars, or having invalid characters.

Some value of non-default IDP which is equal to the IDP-ID of the token

Some value of non-default IDP which is NOT equal to the IDP-ID of the token

Default IDP (i.e. identity_provider_id was an empty string)

What is important to know with the authorization rules we have introduced:

A participant_admin may provide any arbitrary value as IDP-ID (i.e. requestIdentityProviderId does not matter) and authorization layer should not be restricting the admin (that is why if claims.identityProviderId == Default it is giving Right(())

An invalid IdentityProviderId, i.e. non-LedgerString, would give None for requestIdentityProviderId and authorization layer would allow to pass the filter (but currently only for non-Default IDP), as this is nothing to do with PERMISSION_DENIED error which can be issued here.

You're right with your statement that None gives InvalidIdentityProviderId. So PERMISSION_DENIED will be issued in the case of non-default IDP. I think I just have to fix it and pass it further to the business logic layer.

I think in general this highlights a bit broken design of the authorization layer. I don't like that in case of invalid request I'm passing things further and let it fail on the business logic. But doing it differently would break current pattern and will be much more work. Do you maybe have suggestions to improve that?

I would include parts of the justification in form of comment. Surely it is not the last time we will be looking at this code asking same questions as Pawel has

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/Authorizer.scala

ledger-api/grpc-definitions/com/daml/ledger/api/v1/admin/party_management_service.proto

...-auth/src/main/scala/com/digitalasset/ledger/api/auth/IdentityProviderAwareAuthService.scala

...gration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala

...egration-api/src/main/scala/platform/apiserver/services/admin/ApiUserManagementService.scala

...ocal-store/src/main/scala/com/daml/platform/localstore/api/IdentityProviderConfigStore.scala

skisel-da · 2023-01-04T14:10:39Z

What if the user management service is enabled and someone uses a custom daml access token (one doesn't mention the user id). Can such a request succeed?

This should work as before. From the IDP perspective it would be as if request is coming from the default IDP.

pbatko-da

Looks good.
Are you planning to add any tests to this PR? I think it would be worth doing that

ledger-api/grpc-definitions/com/daml/ledger/api/v1/admin/party_management_service.proto

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/AuthorizationError.scala

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/Claims.scala

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/ConfigLoader.scala

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/AuthorizationError.scala

...egration-api/src/main/scala/platform/apiserver/services/admin/ApiUserManagementService.scala

mziolekda · 2023-01-06T13:29:47Z

ledger/metrics/src/main/scala/com/daml/metrics/IdentityProviderConfigStoreMetrics.scala

@@ -16,7 +16,8 @@ class IdentityProviderConfigStoreMetrics(
    override val registry: MetricRegistry,
 ) extends FactoryWithDBMetrics {

-  val cacheByIssuer = new CacheMetrics(prefix :+ "cache_by_issuer", registry)
+  val idpConfigCache = new CacheMetrics(prefix :+ "idp_config_cache", registry)


We need to document these new metrics in UNRELEASED.md

mziolekda · 2023-01-06T13:30:50Z

ledger/participant-integration-api/src/main/scala/platform/LedgerApiServer.scala

+      override def getIdentityProviderConfig(issuer: LedgerId)(implicit
+          loggingContext: LoggingContext
+      ): Future[domain.IdentityProviderConfig] =
+        identityProviderStore.getActiveIdentityProviderByIssuer(issuer)


which execution context should this run on?

servicesExecutionContext ?

I would think so too

...gration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala

…_management_service.proto Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>

…api/auth/AuthorizationError.scala Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>

…server/services/admin/ApiPartyManagementService.scala Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>

skisel-da force-pushed the skisel-da/identity-management-poc4 branch 4 times, most recently from 85c874d to a7d6e96 Compare December 18, 2022 16:50

skisel-da changed the title ~~Identity Provider Config management rebased POC 4~~ Authorization rule changes for IDPs isolation Dec 20, 2022

skisel-da force-pushed the skisel-da/identity-management-poc4 branch from a7d6e96 to 81aced5 Compare December 20, 2022 12:55

skisel-da marked this pull request as ready for review December 20, 2022 12:57

skisel-da requested review from ray-roestenburg-da, a team and meiersi-da as code owners December 20, 2022 12:57

pbatko-da reviewed Dec 21, 2022

View reviewed changes

skisel-da changed the title ~~Authorization rule changes for IDPs isolation~~ Authorization rule changes for IDPs isolation [DPP-1336] Jan 3, 2023

skisel-da force-pushed the skisel-da/identity-management-poc4 branch from 1b42a24 to 9d23bd0 Compare January 5, 2023 11:07

pbatko-da approved these changes Jan 5, 2023

View reviewed changes

skisel-da force-pushed the skisel-da/identity-management-poc4 branch from 90c1c34 to a0c09a7 Compare January 5, 2023 18:21

mziolekda reviewed Jan 6, 2023

View reviewed changes

ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/api/auth/AuthorizationError.scala Outdated Show resolved Hide resolved

mziolekda reviewed Jan 6, 2023

View reviewed changes

...egration-api/src/main/scala/platform/apiserver/services/admin/ApiUserManagementService.scala Outdated Show resolved Hide resolved

mziolekda reviewed Jan 6, 2023

View reviewed changes

...gration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala Outdated Show resolved Hide resolved

mziolekda approved these changes Jan 6, 2023

View reviewed changes

skisel-da added 7 commits January 12, 2023 11:43

Identity Provider Config management rebased POC

caa2fcf

improve authorization rules

18abddd

restrict users

2f85f02

reimplement checking

8867126

additions

a2ecdc7

rollback non-required changes

7966f57

remove todo

1a091a0

skisel-da and others added 24 commits January 12, 2023 11:43

compilation fixes after rebase

98f13d0

move to the package

d37f284

move to the package

40a7594

small changes

79b7f3d

simplifications

3b247df

additional comments and agreements

12c1b8a

productionize the code further

b931332

ammend the test

db87252

fix the tests

fb6b82f

fix the tests

4613434

Blinding rules for party implementation

a9efdb3

Remove the config for testing

67823d7

code review comments

cb49a50

code review comments

8ea7605

code review comments

ede1bdb

code review comments

5af1abd

code review comments

3d97333

rename

957b754

address code review concerns

eb16e2f

Update ledger-api/grpc-definitions/com/daml/ledger/api/v1/admin/party…

2faf496

…_management_service.proto Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>

Update ledger/ledger-api-auth/src/main/scala/com/digitalasset/ledger/…

ece830b

…api/auth/AuthorizationError.scala Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>

Update ledger/participant-integration-api/src/main/scala/platform/api…

b715076

…server/services/admin/ApiPartyManagementService.scala Co-authored-by: mziolekda <marcin.ziolek@digitalasset.com>

another comment

f798a06

method rename as suggested by Marcin

946ad62

skisel-da force-pushed the skisel-da/identity-management-poc4 branch from 0d0f3e9 to 946ad62 Compare January 12, 2023 10:44

updating unreleased.md

b0538c3

skisel-da merged commit f184892 into main Jan 12, 2023

skisel-da deleted the skisel-da/identity-management-poc4 branch January 12, 2023 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorization rule changes for IDPs isolation [DPP-1336] #15848

Authorization rule changes for IDPs isolation [DPP-1336] #15848

skisel-da commented Dec 9, 2022

pbatko-da left a comment

pbatko-da Dec 21, 2022

skisel-da Jan 3, 2023

pbatko-da Jan 3, 2023

skisel-da Jan 4, 2023

mziolekda Jan 6, 2023

skisel-da commented Jan 4, 2023

pbatko-da left a comment •

edited

Loading

mziolekda Jan 6, 2023

mziolekda Jan 6, 2023

skisel-da Jan 6, 2023

mziolekda Jan 6, 2023

Authorization rule changes for IDPs isolation [DPP-1336] #15848

Authorization rule changes for IDPs isolation [DPP-1336] #15848

Conversation

skisel-da commented Dec 9, 2022

pbatko-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skisel-da commented Jan 4, 2023

pbatko-da left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbatko-da left a comment •

edited

Loading