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

Make HTTP and BTP outgoing tokens symmetric to incoming tokens #553

Merged
merged 14 commits into from Dec 5, 2019

Conversation

@gakonst
Copy link
Member

gakonst commented Dec 4, 2019

  1. Mounts /ilp to /accounts/:username/ilp
  2. Mounts /ilp/btp to /accounts/:username/ilp/btp
  3. This allows moving the username from the outgoing_token to being specified at the URL when creating an account in BTP and HTTP
  4. Removes the need for AuthToken in HTTP & BTP tokens.
@gakonst gakonst requested a review from emschwartz as a code owner Dec 4, 2019
crates/interledger-http/src/client.rs Outdated Show resolved Hide resolved
crates/interledger-http/src/server.rs Outdated Show resolved Hide resolved
@gakonst gakonst force-pushed the gakonst/auth-in-username branch from 7a7e036 to 151eb1b Dec 5, 2019
@gakonst gakonst added the breaking label Dec 5, 2019
@gakonst gakonst requested a review from dora-gt as a code owner Dec 5, 2019
@gakonst gakonst force-pushed the gakonst/auth-in-username branch from 5f0dffa to 78303a5 Dec 5, 2019
@gakonst gakonst requested a review from bstrie as a code owner Dec 5, 2019
@@ -81,7 +81,7 @@ jobs:
- run:
name: Run run-md Test
command: |
scripts/run-md-test.sh
scripts/run-md-test.sh '^.*$' 1

This comment has been minimized.

Copy link
@emschwartz

emschwartz Dec 5, 2019

Member

What does that do?

This comment has been minimized.

Copy link
@gakonst

gakonst Dec 5, 2019

Author Member

It runs the Markdown tests in source mode. Otherwise, the tests try to download the binary from the latest release, which is out of date.

@@ -167,15 +170,12 @@ where
.and(warp::header::<String>("authorization"))

This comment has been minimized.

Copy link
@emschwartz

emschwartz Dec 5, 2019

Member
Suggested change
.and(warp::header::<String>("authorization"))
.and(warp::header::<SecretString>("authorization"))

This comment has been minimized.

Copy link
@gakonst

gakonst Dec 5, 2019

Author Member

Not possible currently as we're missing a FromStr implementation for SecretString, opened a PR: iqlusioninc/crates#309

Tracked in #556

crates/interledger-api/src/routes/accounts.rs Outdated Show resolved Hide resolved
.header("authorization", auth.to_bearer())
.header(
"authorization",
&format!("Bearer {}", token.expose_secret()),

This comment has been minimized.

Copy link
@emschwartz

emschwartz Dec 5, 2019

Member

This creates a new String that may not be zeroed in the way that secrecy does it. That's why I thought we might want to make the account method return a SecretString that contains the full authorization header value (including the Bearer part)

@@ -22,7 +23,7 @@ pub use self::server::HttpServer;

pub trait HttpAccount: Account {
fn get_http_url(&self) -> Option<&Url>;
fn get_http_auth_token(&self) -> Option<&str>;
fn get_http_auth_token(&self) -> Option<SecretString>;

This comment has been minimized.

Copy link
@emschwartz

emschwartz Dec 5, 2019

Member

Could this be Option<&SecretString> so it doesn't need to copy it each time?

This comment has been minimized.

Copy link
@gakonst

gakonst Dec 5, 2019

Author Member

This function creates a string which it owns with str::from_utf8 and to_string() subsequently so we cannot really make it return a reference, because it'd be returning a referecne to data it already owns.

str::from_utf8(s.expose_secret().as_ref())

We cannot convert the SecretBytes to a SecretString, because tokens are stored encrypted as SecretBytes and we're storing both encrypted and cleartext values in the same variable.

crates/interledger-http/src/server.rs Show resolved Hide resolved
.as_ref()
.map(|s| str::from_utf8(s.expose_secret().as_ref()).unwrap_or_default())
fn get_http_auth_token(&self) -> Option<SecretString> {
self.ilp_over_http_outgoing_token.as_ref().map(|s| {

This comment has been minimized.

Copy link
@emschwartz

emschwartz Dec 5, 2019

Member

Seems like it should be fine to just return &self.ilp_over_http_outgoing_token

This comment has been minimized.

Copy link
@gakonst

gakonst Dec 5, 2019

Author Member

Tokens are stored as SecretBytes because we use the same variable for both unencrypted and encrypted tokens (which have to be bytes, and cannot be strings).

Hence, this conversion is necessary

@gakonst gakonst changed the title Make HTTP outgoing tokens symmetric to incoming tokens Make HTTP and BTP outgoing tokens symmetric to incoming tokens Dec 5, 2019
gakonst added 14 commits Dec 4, 2019
BREAKING CHANGE: The /ilp endpoint will no longer be used

Ref: #521
This fails with 405 Method Not Found, due to some Warp bug. TODO -> Fix
We recover at the top level API
BREAKING CHANGE: The username is now provided via the path, which means that we do not need to use the username in the btp communication any more
The token is converted to a SecretString to ensure we do not log it
anywhere by accident

Ref: #521
@gakonst gakonst force-pushed the gakonst/auth-in-username branch from 2d38790 to d616bf5 Dec 5, 2019
@gakonst gakonst merged commit 3b6d655 into master Dec 5, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-md Your tests passed on CircleCI!
Details
@gakonst gakonst deleted the gakonst/auth-in-username branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.