Optional secret for Assignable CapAccess #3859

maackle · 2024-05-13T18:34:10Z

Summary

This came out of a documentation discussion. Doesn't this seem preferable?

Also looks like there was a bug, which this fixes

TODO:

CHANGELOGs updated with appropriate info

maackle · 2024-05-13T18:38:12Z

crates/holochain_integrity_types/src/capability/grant.rs

-                    CapAccess::Transferable { secret, .. } => check_secret.map(|given| secret == given).unwrap_or(false),
-                    CapAccess::Assigned { secret, .. } => check_secret.map(|given| secret == given).unwrap_or(false),


I think this was a bug. If check_secret is None, then this function would always just return false, but actually it should have the effect of skipping the secret check. Not sure if we ever use this code path but my change seems correct.

I see this function referenced a few times throughout the codebase, and the app API at least accepts an optional secret, so I imagine it would hit this code path. But wouldn't you need it to return false for a None secret if it's a transferrable grant, or unwrap_or(secret.is_none()) if it's an assigned grant?

(an interesting edge case question is -- what if a secret is supplied and the grant doesn't need one? Should it be true still, even if it doesn't match?)

Well, if this function named is_valid is being called for a Transferable cap grant but with check_secret == None then the result cannot really be represented with a boolean in the first place, can it? It's rather an "undetermined" state - so maybe the function should be called is_validated instead (small joke ;-) )?
So I see how one could reason to return false if no secret is provided for a Transferable cap grant to protect against this function accidentally being called for a transferable grant without providing the secret.

@maackle after reading your comment in light of @matthme 's comment I think I see what you're saying. The effect of skipping the check would be that the loop that checks all the grants on the source chain would also come up with nothing and return false, right? I'm wondering when are the times that the secret would be None -- AFAICT it's only when the caller didn't supply one, in which case I think this check should probably return false rather than getting skipped (although the effect is the same -- access denied).

If check_secret is None, then this function would always just return false

For unrestricted cap grants it returns true, and the other two variants required a secret, so you couldn't skip the secret check. It seemed correct to me.

If the secret becomes optional for assigned cap grants, the logic can no longer be used. But the update is wrong, because a transferable cap grant still requires a secret, and now if none is provided, the secret check is skipped.

this check should probably return false rather than getting skipped (although the effect is the same -- access denied).

In order to properly implement 'skipped' logic in a way that doesn't let SourceChain::valid_cap_grant allow unauthorised people access a function by supplying a None secret, this function would have to have a return signature of Option<bool> and let its consumer treat None as equivalent to false in the loop... (which I guess is what you're saying @matthme ) I'd just as soon return false if a required secret wasn't supplied.

I made the mistake of interpreting check_secret as an imperative i.e. if it's Some, we want to check the secret, if it's None, we don't want to check the secret, and it's the responsibility of the caller to decide whether they want to check the secret or not. I did this too casually, looking more deeply I see that check_secret comes directly from the zome caller, so we can't give them that responsibility.

Let me try again.

I updated this, check my work

LGTM. It would be great to have tests for this method. Are you willing to add them? Otherwise I would.

Test written.

pdaoust · 2024-05-13T19:53:35Z

crates/holochain_integrity_types/src/capability/grant.rs

-                    CapAccess::Transferable { secret, .. } => check_secret.map(|given| secret == given).unwrap_or(false),
-                    CapAccess::Assigned { secret, .. } => check_secret.map(|given| secret == given).unwrap_or(false),


I see this function referenced a few times throughout the codebase, and the app API at least accepts an optional secret, so I imagine it would hit this code path. But wouldn't you need it to return false for a None secret if it's a transferrable grant, or unwrap_or(secret.is_none()) if it's an assigned grant?

(an interesting edge case question is -- what if a secret is supplied and the grant doesn't need one? Should it be true still, even if it doesn't match?)

pdaoust · 2024-05-13T20:00:55Z

crates/holochain_zome_types/src/fixt.rs

@@ -359,7 +359,7 @@ fixturator!(
            CapAccessVariant::Unrestricted => CapAccess::from(()),
            CapAccessVariant::Transferable => CapAccess::from(CapSecretFixturator::new_indexed(Empty, get_fixt_index!()).next().unwrap()),
            CapAccessVariant::Assigned => CapAccess::from((
-                CapSecretFixturator::new_indexed(Empty, get_fixt_index!()).next().unwrap(),
+                Some(CapSecretFixturator::new_indexed(Empty, get_fixt_index!()).next().unwrap()),


Would it be more semantically correct to use None for an empty fixture? (I have no clue; just applying beginner's mind here)

Yeah, that would probably be better

pdaoust · 2024-05-13T20:10:30Z

crates/holochain_zome_types/src/fixt.rs

@@ -376,7 +376,7 @@ fixturator!(
                let number_of_assigned = rng.gen_range(0..5);

                CapAccess::from((
-                    CapSecretFixturator::new_indexed(Unpredictable, get_fixt_index!()).next().unwrap(),
+                    Some(CapSecretFixturator::new_indexed(Unpredictable, get_fixt_index!()).next().unwrap()),


Likewise, wondering if None would also be a valid point in the unpredictable curve.

It would, but it's onerous to make an fixturator for an Option of something, so I opted to just replicate the previous behavior.

got it. Even knowing nothing about how fixt works, I kinda anticipated that it would be complicated.

jost-s · 2024-05-13T22:43:21Z

crates/holochain_integrity_types/src/capability/grant.rs

-                    CapAccess::Transferable { secret, .. } => check_secret.map(|given| secret == given).unwrap_or(false),
-                    CapAccess::Assigned { secret, .. } => check_secret.map(|given| secret == given).unwrap_or(false),


If check_secret is None, then this function would always just return false

For unrestricted cap grants it returns true, and the other two variants required a secret, so you couldn't skip the secret check. It seemed correct to me.

If the secret becomes optional for assigned cap grants, the logic can no longer be used. But the update is wrong, because a transferable cap grant still requires a secret, and now if none is provided, the secret check is skipped.

jost-s · 2024-05-13T22:44:11Z

crates/holochain_integrity_types/src/capability/grant.rs

+                            // Unless the extern is unrestricted.
+                            CapAccess::Unrestricted => true,
+                            // note the PartialEq implementation is constant time for secrets
+                            CapAccess::Transferable { secret, .. } => secret == given_secret,


Transferable requires a secret, the check must not be skipped by not providing one.

pdaoust

Some positive feedback and a non-blocking suggestion.

pdaoust · 2024-05-14T18:05:54Z

crates/holochain_integrity_types/src/capability/grant.rs

-        check_function: &GrantedFunction,
-        check_agent: &AgentPubKey,
-        check_secret: Option<&CapSecret>,
+        given_function: &GrantedFunction,


I like this renaming. Makes the intent of the code much clearer -- I was having to refer back a few times when I was reading it before.

crates/holochain_integrity_types/src/capability/grant.rs

…-optional-secret

jost-s

Yay!

It makes me doubt that passing in a wrong secret when none is required is valid. Fine anyway.

jost-s · 2024-05-20T13:01:34Z

crates/holochain_integrity_types/src/capability/grant.rs

+        let secret_wrong: CapSecret = [2; 64].into();
+        let tag = "tag".to_string();
+
+        let g1: CapGrant = ZomeCallCapGrant {


Suggested change

let g1: CapGrant = ZomeCallCapGrant {

let transferable: CapGrant = ZomeCallCapGrant {

jost-s · 2024-05-20T13:03:31Z

crates/holochain_integrity_types/src/capability/grant.rs

+            functions: GrantedFunctions::All,
+        }
+        .into();
+        let g2: CapGrant = ZomeCallCapGrant {


Suggested change

let g2: CapGrant = ZomeCallCapGrant {

let assigned_to_agent_1_no_secret: CapGrant = ZomeCallCapGrant {

jost-s · 2024-05-20T13:05:13Z

crates/holochain_integrity_types/src/capability/grant.rs

+            functions: GrantedFunctions::All,
+        }
+        .into();
+        let g3: CapGrant = ZomeCallCapGrant {


Suggested change

let g3: CapGrant = ZomeCallCapGrant {

let assigned_to_agent_1_with_secret: CapGrant = ZomeCallCapGrant {

…et' into cap-grant-assigned-optional-secret

…-optional-secret

…et' into cap-grant-assigned-optional-secret

maackle · 2024-05-21T18:35:29Z

@pdaoust want to re-review?

needs re-review

Optional secret for Assignable CapAccess

ea868d0

maackle commented May 13, 2024

View reviewed changes

maackle requested review from pdaoust, neonphog, jost-s and matthme May 13, 2024 18:40

pdaoust previously requested changes May 13, 2024

View reviewed changes

jost-s requested changes May 13, 2024

View reviewed changes

maackle added 2 commits May 14, 2024 09:48

Fix bugfix

e26f5e1

None fixturators

6a53460

pdaoust reviewed May 14, 2024

View reviewed changes

maackle added 3 commits May 17, 2024 16:52

Merge remote-tracking branch 'origin/develop' into cap-grant-assigned…

c8012ee

…-optional-secret

Add cap_grant_is_valid test

a1ddfba

Changelog

2f7d7af

maackle requested review from pdaoust and jost-s May 20, 2024 03:33

jost-s approved these changes May 20, 2024

View reviewed changes

maackle added 7 commits May 20, 2024 14:35

Fix

6b55f66

Fix

cfdaae5

Bump timeout

a52a718

Merge remote-tracking branch 'origin/cap-grant-assigned-optional-secr…

2eda7b9

…et' into cap-grant-assigned-optional-secret

Merge remote-tracking branch 'origin/develop' into cap-grant-assigned…

a94d18d

…-optional-secret

Merge remote-tracking branch 'origin/develop' into cap-grant-assigned…

8d4be14

…-optional-secret

Merge remote-tracking branch 'origin/cap-grant-assigned-optional-secr…

3a8e58a

…et' into cap-grant-assigned-optional-secret

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional secret for Assignable CapAccess #3859

Optional secret for Assignable CapAccess #3859

maackle commented May 13, 2024 •

edited

maackle May 13, 2024

pdaoust May 13, 2024

matthme May 13, 2024

pdaoust May 13, 2024

jost-s May 13, 2024

pdaoust May 14, 2024

maackle May 14, 2024

maackle May 14, 2024

jost-s May 14, 2024

maackle May 18, 2024

pdaoust May 13, 2024

pdaoust May 13, 2024

maackle May 14, 2024

pdaoust May 13, 2024

maackle May 14, 2024

pdaoust May 14, 2024

jost-s May 13, 2024

jost-s May 13, 2024

pdaoust left a comment

pdaoust May 14, 2024

jost-s left a comment

jost-s May 20, 2024

jost-s May 20, 2024

jost-s May 20, 2024

maackle commented May 21, 2024

		CapAccess::Transferable { secret, .. } => check_secret.map(\|given\| secret == given).unwrap_or(false),
		CapAccess::Assigned { secret, .. } => check_secret.map(\|given\| secret == given).unwrap_or(false),

	let g1: CapGrant = ZomeCallCapGrant {
	let transferable: CapGrant = ZomeCallCapGrant {

	let g2: CapGrant = ZomeCallCapGrant {
	let assigned_to_agent_1_no_secret: CapGrant = ZomeCallCapGrant {

	let g3: CapGrant = ZomeCallCapGrant {
	let assigned_to_agent_1_with_secret: CapGrant = ZomeCallCapGrant {

Optional secret for Assignable CapAccess #3859

Are you sure you want to change the base?

Optional secret for Assignable CapAccess #3859

Conversation

maackle commented May 13, 2024 • edited

Summary

TODO:

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

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

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

pdaoust left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jost-s 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

maackle commented May 21, 2024

maackle commented May 13, 2024 •

edited