Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added convenience methods to credentials. #412

Merged
merged 4 commits into from
May 15, 2024

Conversation

Randy808
Copy link
Collaborator

No description provided.

@nepet
Copy link
Collaborator

nepet commented Apr 18, 2024

@Randy808 you may want to rebase this before you put any more work into it, some parts changed, e.g. We reintroduced the fn load_file_or_default(varname: &str, default: &[u8]) -> Result<Vec<u8>> for the CA_CRT and the nobody key/cert pair.

@Randy808 Randy808 force-pushed the credentials-convenience-methods branch 3 times, most recently from 0291adf to 877515c Compare April 19, 2024 02:55
@Randy808 Randy808 mentioned this pull request Apr 22, 2024
4 tasks
@Randy808 Randy808 marked this pull request as ready for review April 22, 2024 10:20
Copy link
Collaborator

@nepet nepet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again! Nice work @Randy808

One thing that I noticed: I think we can get rid of providing the node_id to the scheduler on scheduler::new() altogether. We could add it to the Device credentials e.g. as a trait NodeIdProvider instead of deriving it from the tls_config. The non-authenticated methods register, recover could get the node_id from the signer that they get. This may allow us to completely remove get_node_id_from_tls_config.

What do you think?

Also: We may consider adding with_identity only to the Nobody credentials to avoid any confusion for developers about the use of the Device credentials that should be created from bytes or path.
I am not sure if it would make sense to change the signature of Nobody::with to use an Option for the ca param instead that falls back to `load_file_or_default. Let me know what you think about this.

libs/gl-client/src/credentials.rs Outdated Show resolved Hide resolved
libs/gl-client/src/credentials.rs Outdated Show resolved Hide resolved
libs/gl-client/src/credentials.rs Outdated Show resolved Hide resolved
libs/gl-client/src/credentials.rs Outdated Show resolved Hide resolved
libs/gl-client/src/scheduler.rs Outdated Show resolved Hide resolved
@cdecker
Copy link
Collaborator

cdecker commented Apr 25, 2024

I see a couple of discussions still ongoing, shall we draft this until the discussion has settled, or are these things that we should spin out?

@Randy808 Randy808 marked this pull request as draft April 25, 2024 15:27
@Randy808 Randy808 force-pushed the credentials-convenience-methods branch 2 times, most recently from 6fd4f72 to cd1da65 Compare May 6, 2024 22:48
@Randy808 Randy808 marked this pull request as ready for review May 13, 2024 11:52
@Randy808 Randy808 force-pushed the credentials-convenience-methods branch from 8c3b847 to 9e6acde Compare May 13, 2024 11:53
@cdecker
Copy link
Collaborator

cdecker commented May 14, 2024

Looks like this is runs into the cln-vm-manager issue that @ErikDeSmedt is addressing in #442. I will rebase once that's merged and merge this asap.

@cdecker cdecker force-pushed the credentials-convenience-methods branch from 9e6acde to 400fc43 Compare May 14, 2024 10:24
@cdecker
Copy link
Collaborator

cdecker commented May 15, 2024

ACK 400fc43

@cdecker cdecker merged commit 5c67d50 into Blockstream:main May 15, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants