feat: Add node identity

[islamaliev]

Relevant issue(s)

Resolves #2908

Description

Assign an identity to the node upon upon startup.

[shahzadlone]

Some questions that might be worth it to have in the PR description for reviewers:

question: are users expected to save the node key after it's generated? How exactly is it to be used by users?

question: can a custom private key be given instead by the user initiating the first start. Can node identity be changed after a start?

question: How is the node identity different from the requesting identity (--identity input)?

[codecov]

Codecov Report

Attention: Patch coverage is 53.40909% with 123 lines in your changes missing coverage. Please review.

Project coverage is 77.40%. Comparing base (bb0917e) to head (f3ca301).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
cli/start.go	1.25%	79 Missing ⚠️
acp/identity/raw_identity.go	53.85%	12 Missing ⚠️
acp/identity/identity.go	75.00%	6 Missing and 3 partials ⚠️
cli/node_identity.go	79.31%	5 Missing and 1 partial ⚠️
cli/utils.go	45.45%	4 Missing and 2 partials ⚠️
http/client.go	50.00%	4 Missing and 2 partials ⚠️
http/handler_store.go	85.19%	3 Missing and 1 partial ⚠️
internal/db/db.go	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3125      +/-   ##
===========================================
- Coverage    78.05%   77.40%   -0.65%     
===========================================
  Files          354      357       +3     
  Lines        34672    34809     +137     
===========================================
- Hits         27060    26942     -118     
- Misses        5988     6250     +262     
+ Partials      1624     1617       -7     

Flag	Coverage Δ
all-tests	77.40% <53.41%> (-0.65%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
acp/identity/context.go	100.00% <100.00%> (ø)
acp/identity/generate.go	50.00% <100.00%> (-12.50%)	⬇️
cli/acp_relationship_add.go	100.00% <100.00%> (ø)
cli/acp_relationship_delete.go	100.00% <100.00%> (ø)
cli/cli.go	100.00% <100.00%> (ø)
client/db.go	89.29% <ø> (ø)
http/auth.go	65.62% <100.00%> (ø)
http/errors.go	28.00% <ø> (ø)
http/http_client.go	72.73% <100.00%> (ø)
http/openapi.go	97.52% <ø> (ø)
... and 18 more

... and 24 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb0917e...f3ca301. Read the comment docs.

[nasdf]

Looks good overall, just a few minor things to discuss.

[jsimnz]

Quick Todo. Also commented on another Todo regarding logging of identity. In general, you should be extremely careful when touching the private key for any I/O related actions. Should only be touched for actual cryptographic ops

[AndrewSisley]

Looks good to me, just couple of small suggestions from me :)

[nasdf]

Looks good!

[AndrewSisley]

todo: This still feels out of step with how I thought it should behave, especially given the 10 year expiry on the jws token.

I think the identity.Identity should be constructed within db.GetNodeIdentity, with a 15min (default) expiry, the only thing held in state on the db object should be the keyring/name-of-key-in-keyring (could be const for now).

Very much worth checking in with the rest of the team on this in case I'm talking nonsense :)

[shahzadlone]

suggestion: I would like to avoid this if possible.

[islamaliev]

But we want to differenciate between node identities and externally provided udentities. How else do you want to do it?
Yes, this make it more verbose, but explicit at the same time. You like "explicit", don't you? :)

[shahzadlone]

But we want to differenciate between node identities and externally provided identities. How else do you want to do it?

I thought I described my prefer way to do it here already and that you were okay with it? #3125 (comment)

In short, leave previous identities the way they are and make a new one for node Identities rather than trying to have to both fit the same mold.

[islamaliev]

You mean to add to every test action an additional field? Like:

Identity immutable.Option[int]
NodeIdentity immutable.Option[int]

I don't think it's good design. First of all, you can't have both of them set, you will have to document every action and check on executtion of this action if not more then 1 is set. Another point is, what if in the future we add another type of identity? That's going to hurt.

And anyway all identities we used so far ARE node identities, beucase they are attached to a node. So it should be:

Identity immutable.Option[int]
UserIdentity immutable.Option[int]

[shahzadlone]

That is not what I am proposing.

I am saying leave all actions that existed before to use the identity implementation that existed before and not touch them, all tests should remain the way they were. Just rename the identities [][]identity.Identity on state struct (i.e. s.idenities) to requestingIdentities (or userIdentities) and add a new variable to the state struct called nodeIdentities (or whatever you would like to call it, and make the implementation you have introduced in this PR for example 1D array, do all of that for that identity).

Then only the test.cases that use node identity stuff can just use that new variables identity logic for example: testUtils.GetNodeIdentity action can operate over just the nodeIdentities and doesn't need to worry about the other identities logic the way we had it before.

This way you don't need the new custom IdentRef and know for sure each action case is operating on it's domain of identities just like you said you can't have both of them set at once, so I am very much agains't mixing these.

[fredcarle]

There is a problem with keeping things as is because when we start needing to set node identities to docs and/or policies, things will need to be reworked anyways.

One thing I like about the test changes in this PR is the decoupling of identity generation from token generation. It has the potential to keep things cleaner.

I think the identRef approach could be improved though and I have a few ideas to simplify things and make both Shahzad and Islam happy with the outcome. We can discuss during standup.

[shahzadlone]

There is a problem with keeping things as is because when we start needing to set node identities to docs and/or policies, things will need to be reworked anyways.

What do you mean by setting node identity to docs/policy? I thought node identity is an identity that will be set once for a node on start (if an admin user chooses to ofc).

One thing I like about the test changes in this PR is the decoupling of identity generation from token generation. It has the potential to keep things cleaner.

I like the decoupling as well (specially for the type of identity that needs it).

I think the identRef approach could be improved though and I have a few ideas to simplify things and make both Shahzad and Islam happy with the outcome. We can discuss during standup.

I think a middle approach can be to make an optional type that takes a custom type containing the index and the type of identity (i.e. remove the hasValue and value type from the custom type). However I just still fail to quite understand why we have to couple both types of identities together

[shahzadlone]

question: Curious why this was done?

[islamaliev]

what exactly? We need a context with an identity in it for sending a request.
Please clarify your question. I also documented some of my decisions. Let me know, if they are not clear enough.

[shahzadlone]

Sorry, I will clarify. I am wondering why the function couldn't access s.ctx when the function is consuming the s already in the function argument. Why did we need to have a new parameter and pass s.ctx separately?

[islamaliev]

because in some parts of the code the context is not stored in s.ctx. I don't know if there is a good reason for this.
I can actually change it to something like this:

s.ctx = newCtx
updateContextWithIdentity(s, action.Identity, nodeIDs[index]) // will store in s.ctx
policyResult, err := node.AddPolicy(s.ctx, action.Policy)

Not sure if that makes it better, but I don't have a strong opinion here.

[shahzadlone]

because in some parts of the code the context is not stored in s.ctx. I don't know if there is a good reason for this. I can actually change it to something like this:

s.ctx = newCtx
updateContextWithIdentity(s, action.Identity, nodeIDs[index]) // will store in s.ctx
policyResult, err := node.AddPolicy(s.ctx, action.Policy)

Not sure if that makes it better, but I don't have a strong opinion here.

Oh wow, perhaps worth investigating later where and why that happens. I am happy with the suggested solution you gave for places that have missing ctx

[shahzadlone]

question: Why did we want to have the set/assign command as a separate command and not just an option at start?

[islamaliev]

todo: This still feels out of step with how I thought it should behave, especially given the 10 year expiry on the jws token.

I think the identity.Identity should be constructed within db.GetNodeIdentity, with a 15min (default) expiry, the only thing held in state on the db object should be the keyring/name-of-key-in-keyring (could be const for now).

Very much worth checking in with the rest of the team on this in case I'm talking nonsense :)

I removed the duration. And actually I removed node-identity assign command altogether.

Posting my quote from discord with explanation

initially I created it, because I stumbled upon a problem in our testing framework. It was not possible to provide an identity on startup, because it required audience first which could be retrieved only after a node is created. Now thinking about it after I split identity creation and token generation, it might be possible. I will check it again. Maybe we don't need it anymore.

[AndrewSisley]

Looks really good Islam - sorry the review stage was a bit of a drag - I think we as a team failed here a little bit and that we all could have saved a fair amount of effort by paying more attention earlier on and discussing things before code was written.

Please make sure the conversation RE testutils internal stuff with Fred is resolved before merge. My personal opinion on it is that the difference is pretty small and delaying the merge is probably more costly than achieving consensus on what the the perfect solution is there. Fred's suggestion looks pretty easy to do post-merge too (no test changes).

[shahzadlone]

Thanks for bearing with the review.

I do prefer fred's approach as it is very close to what I was proposing: 62154fa#diff-b4cf51ec3aeceed911432510677147fd0d93a5c02680a4efce39566bfa8ebeb0

It's not worth blocking the PR over though so I am leaving an LGTM :)

nitpick: bit more descriptive PR title, perhaps: feat: Add Node Identity

inactive