feat: Add authentication for ACP #2649

nasdf · 2024-05-25T18:46:49Z

Relevant issue(s)

Resolves #2017

Description

This PR adds ACP identity authentication via HTTP.

Notable changes:

acp/identity has been replaced with the acp.Identity struct
- identity.PrivateKey is the private key of the identity
- identity.PublicKey is the public key of the identity
- identity.Address is the bech32 formatted address for the identity
- keys are all secp256k1
http can authenticate requests using a jwt bearer token
- ~~a random audience value is generated on every http server startup~~
- audience must be set to the defradb host name
- ~~api route /audience returns the random audience value~~
- http.Client will create a signed token if an acp.PrivateKeyIdentity is set
- jwt token subject is the identity public key
cli --identity flag is now a hex encoded private key

Todo:

ensure acp docs are updated

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

AndrewSisley

Looks good, but I have a handful of questions/todos for you at the moment :)

acp/identity.go

http/handler.go

tests/integration/acp/add_policy/with_invalid_creator_arg_test.go

http/auth_test.go

acp/identity.go

tests/integration/acp/add_policy/with_empty_args_test.go

acp/identity/identity.go

acp/identity.go

shahzadlone

Did a first pass quickly, left some todos, will review again tonight.

Can you please also update the acp readme with authentication cli examples now that we will always have acp with authentication. It will help better understand for reviewers as well, how acp with auth works.

acp/identity.go

crypto/crypto.go

shahzadlone · 2024-05-27T18:54:05Z

crypto/crypto.go

 )

+// GenerateSecp256k1 generates a new secp256k1 private key.
+func GenerateSecp256k1() (*secp256k1.PrivateKey, error) {


question: You only need this for testing right? not for any production code?

Yes, I'm planning on adding a way to generate and store them in the keyring as well.

acp/identity.go

shahzadlone

Some more questions.

Question: Was the ability to send requests without an identity (optionality of an identity being set or specified) removed? IMO we need to maintain optional identity.

Question: If Identity is still optional, when a verification fails, do you error out or assume that no identity was provided and still execute request? IMO should be an error even if optional identity is stripped.

internal/db/db.go

tests/clients/cli/wrapper_cli.go

nasdf · 2024-05-28T21:56:18Z

Question: Was the ability to send requests without an identity (optionality of an identity being set or specified) removed? IMO we need to maintain optional identity.

No

Question: If Identity is still optional, when a verification fails, do you error out or assume that no identity was provided and still execute request? IMO should be an error even if optional identity is stripped.

If verification fails the request returns a 403 status code / forbidden error.

AndrewSisley

Re-requesting changes, you appear to have forgotten about/forgotten to push a few of my previous todos.

http/handler.go

acp/identity/identity.go

AndrewSisley

LGTM, thanks Keenan! I would prefer that you wait on Shahzad's approval before merge though :)

acp/identity/identity.go

cli/acp_policy_add.go

jsimnz · 2024-05-29T22:14:49Z

reviewing :) 👀

jsimnz

Obviously there is on going work with this part of the codebase, approving now so I don't further block the merging. Only one todo, and some thoughts/questions

acp/README.md

jsimnz · 2024-05-29T22:36:18Z

http/handler.go

@@ -74,15 +76,26 @@ func NewHandler(db client.DB) (*Handler, error) {
 	if err != nil {
 		return nil, err
 	}
+	// generate a secure random audience value
+	audienceBytes, err := crypto.RandomBytes(64)


question: Could we use the peerID of the node as the audience, instead of random bytes? At the moment, restarting the node will invalidate previous JWS, since this creates a new audience value every time the node starts, where as the peer-id is initially random (function of nodes peer key) but constant after initialization.

The problem I ran into is that the peer ID can't easily be retrieved when p2p is disabled.

Isn't that a good thing that the new audience value can be assigned to enforce re-validation using new JWS so previous ones can't be used (for whatever reason, some token got compromised or something).

Audience is now the api host name. Will update to include peer ID in a follow up PR.

tests/clients/cli/wrapper_cli.go

shahzadlone

Thanks Keenen for doing the changes. I have some more comments that I have posted, please ping me once ready for re-review, and will give another final review :) Looks really nice now and I am super excited to have this in.

Sorry I forgot to post the action item points from our zoom call yesterday, I know you have done some of those items, but just leaving them here for reference to knock them all down pre-merge.

Explicit empty identity in the tests that it was removed from
Use identity option before checking private key, or re-think the option data struct now that identity must have private before verification and will only have public key after verfication.
Documentation requests:
- Document how to generate hex from private key for CLI
- Document why we use HEX for CLI
- Document HEX type for decoding and encoding
- Document HTTP auth-flow
- Document command using /audiance endpoint/domain for HTTP
- Document how to get the signed token with HTTP
- Document where to put the signed token with HTTP (i.e. Bearer often times people confuse this with GQL Variable haha)

shahzadlone · 2024-05-30T18:59:28Z

acp/README.md

+read EC key
+Private-Key: (256 bit)
+priv:
+    e3:b7:22:90:6e:e4:e5:63:68:f5:81:cd:8b:18:ab:


todo: for now a one liner about removing colons or the command I shared below in the documentation would be nice IMO

shahzadlone · 2024-05-30T19:07:07Z

acp/README.md

+defradb client acp policy add -f examples/dpi_policy/user_dpi_policy.yml \
+--identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac


todo: remove the \ added because now when you copy the command and dump to terminal it will assume there are separate commands, its a readme so avoid the \ and just have the long command so it's simple to copy and paste.

Suggested change

defradb client acp policy add -f examples/dpi_policy/user_dpi_policy.yml \

--identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac

defradb client acp policy add -f examples/dpi_policy/user_dpi_policy.yml --identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac

shahzadlone · 2024-05-30T19:08:34Z

acp/README.md

-defradb client acp policy add -i cosmos1f2djr7dl9vhrk3twt3xwqp09nhtzec9mdkf70j -f examples/dpi_policy/user_dpi_policy.yml
-
+defradb client acp policy add -f examples/dpi_policy/user_dpi_policy.yml \
+--identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac
 ```

 Result:


nitpick: The Results are not true anymore, it will now be:

{ "PolicyID": "50d354a91ab1b8fce8a0ae4693de7616fb1d82cfc540f25cfbe11eb0195a5765" }

Therefore the example in examples/schema/permissioned/users.graphql id will need to update too.

shahzadlone · 2024-05-30T19:29:23Z

cli/collection_delete.go

@@ -28,7 +28,8 @@ Example: delete by docID:
  defradb client collection delete  --name User --docID bae-123

 Example: delete by docID with identity:
-  defradb client collection delete -i cosmos1f2djr7dl9vhrk3twt3xwqp09nhtzec9mdkf70j --name User --docID bae-123
+  defradb client collection delete --name User --docID bae-123 \
+  	-i 028d53f37a19afb9a0dbc5b4be30c65731479ee8cfa0c9bc8f8bf198cc3c075f


nitpick: doesn't the extra trailspace in next line after \ break the bash command if you copy paste into terminal?

It works for me

shahzadlone · 2024-05-30T19:35:14Z

http/auth.go

+	return jwt.NewBuilder().
+		Subject(subject).
+		Audience([]string{audience}).
+		Expiration(time.Now().Add(15 * time.Minute)).


todo: the expire time should be made a const maybe a const on top of this file with authSchemaPrefix, should be easier to make this tweakable in future.

todo: Might be worth documenting the default expiry time of the token is 15 mins somewhere.

shahzadlone · 2024-05-30T19:55:36Z

tests/clients/cli/wrapper_cli.go

@@ -58,8 +59,8 @@ func (w *cliWrapper) executeStream(ctx context.Context, args []string) (io.ReadC
 		args = append(args, "--tx", fmt.Sprintf("%d", tx.ID()))
 	}
 	id := db.GetContextIdentity(ctx)
-	if id.HasValue() {
-		args = append(args, "--identity", id.Value().String())
+	if id.Value().PrivateKey != nil {


suggestion: I would still like this id.HasValue() check before the privateKey not nill check IMO

tests/integration/acp/add_policy/with_empty_args_test.go

shahzadlone · 2024-05-30T19:59:45Z

tests/integration/acp/add_policy/with_invalid_creator_arg_test.go

@@ -57,12 +66,18 @@ func TestACP_AddPolicy_InvalidCreatorIdentityWithValidPolicy_Error(t *testing.T)

 func TestACP_AddPolicy_InvalidCreatorIdentityWithEmptyPolicy_Error(t *testing.T) {
 	test := testUtils.TestCase{
+		// Using an invalid creator is not possible with other client
+		// types as the address is derived from the public key used


question: is there a typo? public key used to signed the jwt or the private key?

shahzadlone · 2024-05-30T22:25:37Z

tests/integration/acp/fixture.go

 )
+
+func init() {


question: Why does this need to be in an init function?

todo: I would much prefer a util function some what like this:

func toIdentity(privateHex string) immutable.Option[acpIdentity.Identity] { privateKeyBytes, err := hex.DecodeString(privateHex) if err != nil { panic(err) } return acpIdentity.FromPrivateKey( secp256k1.PrivKeyFromBytes( privateKeyBytes, ), ) } var ( Actor1Identity = toIdentity("028d53f37a19afb9a0dbc5b4be30c65731479ee8cfa0c9bc8f8bf198cc3c075f") Actor2Identity = toIdentity("4d092126012ebaf56161716018a71630d99443d9d5217e9d8502bb5c5456f2c5") )

suggestion: Something similar can replace the loadIdentity function you have in the other test as well.

shahzadlone · 2024-05-30T22:44:45Z

tests/integration/acp/index/fixture.go

@@ -39,7 +39,7 @@ resources:
          - actor
 `

-// policy id: "68a4e64d5034b8a0565a90cd36483de0d61e0ea2450cf57c1fa8d27cbbf17c2c"
+// policy id: "e3c35f345c844e8c0144d793933ea7287af1930d36e9d7d98e8d930fb9815a4a"


praise: Thanks for maintaining the documentation accuracy here, would also be worth fixing the user facing examples in the example directory and the acp readme as well (I think I mentioned in other comments).

shahzadlone

Awesome work @nasdf appreciate you doing all the changes :)

One last nitpick, before merge if you can change the title of the PR to have the first word as an action verb instead of ACP it would be nice.

For example, feat: Add Authentication for ACP

nasdf added 3 commits May 25, 2024 11:29

add public and private acp identity

497a263

add http auth funcs and middleware

41d5112

update tests

5085640

nasdf added the area/acp Related to the acp (access control) system label May 25, 2024

nasdf added this to the DefraDB v0.12 milestone May 25, 2024

nasdf self-assigned this May 25, 2024

nasdf requested a review from a team May 25, 2024 18:57

AndrewSisley requested changes May 27, 2024

View reviewed changes

AndrewSisley reviewed May 27, 2024

View reviewed changes

acp/identity.go Outdated Show resolved Hide resolved

shahzadlone requested changes May 27, 2024

View reviewed changes

tests/integration/acp/add_policy/with_empty_args_test.go Show resolved Hide resolved

shahzadlone reviewed May 27, 2024

View reviewed changes

acp/identity/identity.go Show resolved Hide resolved

shahzadlone reviewed May 27, 2024

View reviewed changes

acp/identity.go Outdated Show resolved Hide resolved

shahzadlone requested changes May 27, 2024

View reviewed changes

AndrewSisley reviewed May 28, 2024

View reviewed changes

acp/identity.go Outdated Show resolved Hide resolved

flatten identity to singular struct

8f751c8

shahzadlone reviewed May 28, 2024

View reviewed changes

internal/db/db.go Outdated Show resolved Hide resolved

tests/clients/cli/wrapper_cli.go Show resolved Hide resolved

move acp/identity.go to acp/identity/identity.go

92007dc

nasdf and others added 2 commits May 28, 2024 15:07

update cli and acp docs

692a642

Merge branch 'develop' into nasdf/feat/acp-auth

c8c4a85

nasdf requested review from AndrewSisley and shahzadlone May 28, 2024 22:10

AndrewSisley requested changes May 29, 2024

View reviewed changes

nasdf added 2 commits May 29, 2024 09:15

fix invalid creator acp tests

61a00a6

lint fixes

c0d2cd1

nasdf requested a review from AndrewSisley May 29, 2024 16:19

AndrewSisley reviewed May 29, 2024

View reviewed changes

http/handler.go Outdated Show resolved Hide resolved

AndrewSisley reviewed May 29, 2024

View reviewed changes

acp/identity/identity.go Show resolved Hide resolved

log errors from response writers

7628e43

AndrewSisley approved these changes May 29, 2024

View reviewed changes

shahzadlone reviewed May 29, 2024

View reviewed changes

nasdf added 3 commits May 29, 2024 13:26

fix cli documentation lll linter errors

b02c066

add authentication section to ACP readme

9dd09d8

fix acp readme example

38f607f

nasdf requested a review from shahzadlone May 29, 2024 20:59

jsimnz approved these changes May 30, 2024

View reviewed changes

shahzadlone requested changes May 30, 2024

View reviewed changes

nasdf added 5 commits May 31, 2024 10:02

change jwt audience to host

aba0c8f

pr fixes

fc03906

add openapi security schemes

dba0981

update acp authentication documentation

a2e5f11

fix lint error

a51013b

nasdf requested a review from shahzadlone May 31, 2024 18:02

shahzadlone approved these changes May 31, 2024

View reviewed changes

nasdf changed the title ~~feat: ACP authentication~~ feat: Add Authentication for ACP May 31, 2024

nasdf changed the title ~~feat: Add Authentication for ACP~~ feat: Add authentication for ACP May 31, 2024

Merge branch 'develop' into nasdf/feat/acp-auth

b708c38

nasdf merged commit c5ce2f3 into sourcenetwork:develop May 31, 2024
29 of 30 checks passed

fredcarle added the feature New feature or request label Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add authentication for ACP #2649

feat: Add authentication for ACP #2649

nasdf commented May 25, 2024 •

edited

Loading

AndrewSisley left a comment

shahzadlone left a comment

shahzadlone May 27, 2024

nasdf May 28, 2024

shahzadlone left a comment

nasdf commented May 28, 2024

AndrewSisley left a comment

AndrewSisley left a comment

jsimnz commented May 29, 2024

jsimnz left a comment

jsimnz May 29, 2024

nasdf May 30, 2024

shahzadlone May 30, 2024

nasdf May 31, 2024

shahzadlone left a comment

shahzadlone May 30, 2024

shahzadlone May 30, 2024

shahzadlone May 30, 2024

shahzadlone May 30, 2024

shahzadlone May 30, 2024

nasdf May 31, 2024

shahzadlone May 30, 2024

shahzadlone May 30, 2024

shahzadlone May 30, 2024

shahzadlone May 30, 2024

shahzadlone May 30, 2024

shahzadlone left a comment

		defradb client acp policy add -f examples/dpi_policy/user_dpi_policy.yml \
		--identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac

feat: Add authentication for ACP #2649

feat: Add authentication for ACP #2649

Conversation

nasdf commented May 25, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

AndrewSisley left a comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented May 28, 2024

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

jsimnz commented May 29, 2024

jsimnz 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

shahzadlone 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

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

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented May 25, 2024 •

edited

Loading