feat(bindle push): add basic auth support #407

vdice · 2022-04-27T18:06:43Z

Adds basic auth support for bindle push per bindle push: add basic http auth username/password support #305

I see at least two follow-ups if we want to add support for bindle server basic auth across Spin (or, could perhaps be persuaded to tackle one or more in this effort):

~~1. Adding basic auth config for spin deploy~~ added in 633e0cd
2. Updating the loader crate which still appears to use a No Auth approach (For our current tests, this still works because thus far, bindle server allows anonymous GETs even when running with basic auth). See #411

Thoughts?

Closes #305

itowlson

This looks really nice and clean. Some minor suggestions but nothing blocking.

itowlson · 2022-04-28T01:47:07Z

crates/publish/src/bindle_pusher.rs

 ) -> Result<()> {
    let reader = StandaloneRead::new(&path, bindle_id).await?;
    reader
-        .push(client)
+        .push(&bindle_connection_info.client()?)


Might be good to have a context/with_context here - this can help users understand the error

@itowlson Are you envisioning something like this?

.push( &bindle_connection_info.client() .context( format!( "Failed to create a bindle client from provided connection info '{}'", bindle_connection_info.display() ) )? ) .await .with_context(|| push_failed_msg(path, &bindle_connection_info.base_url))

(Where, if so, or if on the right track, I'd add a display() function to BindleConnectionInfo for printing)

Yeah, that sort of thing. Though for readability I would probably pull it out into e.g.

let client = &bci.client().with_context(...); ... .push(&client)

rather than having the massive with_context block breaking up the flow

@itowlson updated in 55c3f95 -- how does this look? Happy to tweak some more...

(I thought it best not to print the entire bci struct as it may contain sensitive info like the un/pw values for basic auth)

Looks great! Thanks!

itowlson · 2022-04-28T01:50:56Z

crates/publish/src/lib.rs

+        Self {
+            base_url: base_url.into(),
+            allow_insecure,
+            token_manager: AnyAuth {


The get side currently uses a different idiom for this (mostly because I foolishly reinvented the wheel). Medium term - outside the scope of this PR - we should replace BindleTokenManager on the get side with AnyAuth.

👍 Agreed. It sounds like it'll be okay to do this work in a follow-up; I've created #411

itowlson · 2022-04-28T01:52:18Z

src/commands/bindle.rs

+    #[structopt(
+        name = BINDLE_HTTP_PASSWORD,
+        long = "bindle-http-password",
+        env = BINDLE_HTTP_PASSWORD,


Should this and BINDLE_HTTP_USER require each other?

Yes, added in bfb7385

itowlson · 2022-04-28T01:53:34Z

src/commands/deploy.rs

-        let client = crate::create_bindle_client(true, &self.bindle_server_url)?;
+        let bindle_connection_info = spin_publish::BindleConnectionInfo::new(
+            &self.bindle_server_url,
+            // TODO: do we want to add insecure and http user/pw opts for this deploy cmd?


It should support the same things bindle push does I think, though could leave out of scope for this PR if you want.

This should be straightforward enough to add in this PR, so I'll do so.

Added in 633e0cd (cc @michelleN )

itowlson · 2022-04-28T01:55:21Z

tests/integration.rs

@@ -81,10 +87,53 @@ mod integration_tests {
        Ok(())
    }

+    #[tokio::test]
+    async fn test_bindle_roundtrip_basic_auth() -> Result<()> {
+        // start the Bindle registry.


Love how convenient the test setup is here.

itowlson · 2022-04-28T01:58:56Z

crates/publish/Cargo.toml

@@ -13,6 +13,7 @@ futures = "0.3.14"
 itertools = "0.10.3"
 mime_guess = { version = "2.0" }
 path-absolutize = "3.0.11"
+reqwest = "0.11"


Is this needed? Not sure why the Bindle client doesn't handle it all for us.

Yea, not sure... without this, I'm seeing:

$ make build cargo build --release Compiling spin-cli v0.1.0 (/Users/vdice/go/src/github.com/fermyon/spin) Compiling spin-publish v0.1.0 (/Users/vdice/go/src/github.com/fermyon/spin/crates/publish) error[E0433]: failed to resolve: use of undeclared crate or module `reqwest` --> crates/publish/src/lib.rs:73:18 | 73 | builder: reqwest::RequestBuilder, | ^^^^^^^ use of undeclared crate or module `reqwest` error[E0433]: failed to resolve: use of undeclared crate or module `reqwest` --> crates/publish/src/lib.rs:74:33 | 74 | ) -> bindle::client::Result<reqwest::RequestBuilder> { | ^^^^^^^ use of undeclared crate or module `reqwest` For more information about this error, try `rustc --explain E0433`. error: could not compile `spin-publish` due to 2 previous errors

But maybe there's another way to go about this?

Eh, I guess it's needed then grin - leave it.

michelleN · 2022-04-28T14:26:47Z

src/commands/bindle.rs

+    #[structopt(
+        name = BINDLE_HTTP_USER,
+        long = "bindle-http-user",
+        env = BINDLE_HTTP_USER,


Not blocking this PR: we should make username and password environment variables consistent across commands. Like spin deploy requires a hippo user name and password and the env vars look a little diff there.

Good point; updated in 1fe2964

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

…test Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

… hippo auth Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

itowlson

LGTM

itowlson · 2022-04-28T21:03:23Z

src/commands/bindle.rs

+        name = BINDLE_USERNAME,
+        long = "bindle-username",
+        env = BINDLE_USERNAME,
+        requires(BINDLE_PASSWORD)


Nit: can we not write requires = ... for consistency? I thought this worked elsewhere but could be wrong.

Oh certainly, good call.

Updated in 6311a2d

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

vdice changed the title ~~Feat/bindle basic auth~~ feat(bindle push): add basic auth support Apr 27, 2022

vdice force-pushed the feat/bindle-basic-auth branch from bf17e2f to 8761d3a Compare April 28, 2022 00:37

radu-matei requested a review from itowlson April 28, 2022 01:36

itowlson approved these changes Apr 28, 2022

View reviewed changes

michelleN approved these changes Apr 28, 2022

View reviewed changes

vdice mentioned this pull request Apr 28, 2022

Refactor: Update loader crate (bindle get) to use AnyAuth #411

Closed

vdice requested review from itowlson and michelleN April 28, 2022 18:28

vdice added 5 commits April 28, 2022 14:34

feat(bindle push): add http basic auth support

939eb25

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

feat(tests): add auth config to BindleTestController; add basic auth …

99471af

…test Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

configure bindle auth params to require each other

f65ca9f

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

update bindle user/pw var names and strings to match pattern used for…

230a20f

… hippo auth Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

feat(deploy): add insecure, bindle-username and bindle-password support

5f23276

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

itowlson approved these changes Apr 28, 2022

View reviewed changes

vdice added 2 commits April 28, 2022 15:26

use 'requires = *' syntax

6311a2d

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

add with_context to bindle client generation

55c3f95

Signed-off-by: Vaughn Dice <vaughn.dice@fermyon.com>

vdice force-pushed the feat/bindle-basic-auth branch from 633e0cd to 55c3f95 Compare April 28, 2022 21:35

vdice merged commit 469925b into fermyon:main Apr 28, 2022

vdice deleted the feat/bindle-basic-auth branch April 28, 2022 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bindle push): add basic auth support #407

feat(bindle push): add basic auth support #407

vdice commented Apr 27, 2022 •

edited

Loading

itowlson left a comment

itowlson Apr 28, 2022

vdice Apr 28, 2022

itowlson Apr 28, 2022

vdice Apr 28, 2022

vdice Apr 28, 2022

itowlson Apr 28, 2022

itowlson Apr 28, 2022

vdice Apr 28, 2022

itowlson Apr 28, 2022

vdice Apr 28, 2022

itowlson Apr 28, 2022

vdice Apr 28, 2022

vdice Apr 28, 2022

itowlson Apr 28, 2022

itowlson Apr 28, 2022

vdice Apr 28, 2022

itowlson Apr 28, 2022

michelleN Apr 28, 2022

vdice Apr 28, 2022

itowlson left a comment

itowlson Apr 28, 2022

vdice Apr 28, 2022

vdice Apr 28, 2022

feat(bindle push): add basic auth support #407

feat(bindle push): add basic auth support #407

Conversation

vdice commented Apr 27, 2022 • edited Loading

itowlson 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

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

itowlson 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

vdice commented Apr 27, 2022 •

edited

Loading