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

rust-client: add api-key & listen_multi_addresses #23

Merged
merged 5 commits into from
Jul 9, 2022

Conversation

elenaf9
Copy link
Collaborator

@elenaf9 elenaf9 commented Jul 5, 2022

Update rust-client to be compatible with the latest main:

  • Add api-key to authenticate the client at the server (Closes Authenticate Clients #13)
  • Adapt to other minor protobuf changes

Note: I was only able to briefly test it again our test server, where it worked as expected. Now not working anymore, most likely because of #22. I will fix this in a separate PR, but might take at bit more time.


Also re-enabled the CI since we are now compatible again, but not insisting on that. @dennis-tra in case that you prefer it being disabled while things are still changing a lot I am happy to keep it commented out.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Just one comment. Otherwise looks good to me. Thanks for tackling this!

Comment on lines 55 to 57
/// Api-key used to authenticate our client at the server.
#[clap(long)]
api_key: String,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should pass a secret via a command line argument. This is problematic due to:

  • The secret ends up in the shell's history.
  • Other users can see the secret via /proc.

Instead I would use either an environment variable or a file where the file path can be passed as an argument.

What do you think @elenaf9?

Copy link
Collaborator Author

@elenaf9 elenaf9 Jul 9, 2022

Choose a reason for hiding this comment

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

Good point. Reading it from env variable now (50253b0). If it is not present I am logging a warning but still continuing, in case that we ever decide to (temporarily?) disable auth again. Can also change it in a follow up PR to throw an error, if you think that makes more sense.

Copy link
Member

Choose a reason for hiding this comment

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

Given that it is a mechanism for the server to establish trust to the client and not the other way around, I think defaulting to not using the API key is fine here. 👍

@dennis-tra
Copy link
Collaborator

Also re-enabled the CI since we are now compatible again, but not insisting on that. @dennis-tra in case that you prefer it being disabled while things are still changing a lot I am happy to keep it commented out.

All good, I just disabled it because it understandably started failing after I did protobuf adjustments :)

@dennis-tra
Copy link
Collaborator

BTW, also LGTM, so feel free to merge :)

@elenaf9 elenaf9 merged commit de7fa8b into libp2p:main Jul 9, 2022
@elenaf9 elenaf9 deleted the rust-client/patch-1 branch July 11, 2022 04:51
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.

Authenticate Clients
3 participants