client: add basic auth support #1

tormath1 · 2021-10-27T07:45:58Z

In this PR, we add the support for Basic Authentication.

Testing done

Only unit testing to assert that the headers are correctly set.

invidian

Do we also plan to plug this feature into the CLI?

What about sending credentials over plain text (HTTP)? Shall we print some warning to the user if this is done, as it might be insecure?

pkg/client/options.go

pkg/client/client.go

invidian

Looks mostly fine, but I'm still curious about the round tripper part.

pkg/client/client.go

pkg/client/options.go

tormath1 · 2021-10-27T17:30:24Z

Looks mostly fine, but I'm still curious about the round tripper part.

I'm curious too - just pushed #2 to implement the RoundTripper logic in the tests and pushed pkg/client: add basic auth round tripper to prepare the basic auth round tripper.

I merged locally #2 with this PR and it works fine, it's easier to test too.

tormath1 · 2021-10-28T09:48:54Z

In the three latest commits we reverted the WithBasicAuth option and updated the basic auth test to use an HTTP Client with a transport based on the basic auth round tripper.

If one wants to create a Fleetlock client, he will need to provide an HTTP Client with client.BasicAuthRoundTripper transport.

@invidian if it's ok with you, let's rebase the commits since the logic has changed a bit.

pkg/client/client_test.go

pkg/client/options.go

tormath1 · 2021-10-28T13:00:49Z

@invidian done :)

use existing httpClient instead of creating a new struct to implement the RoundTripper interface
convert Options to Config

invidian

@invidian done :)

Thanks for addressing it. I think I have few more concerns/suggestions.

invidian · 2021-10-28T13:19:42Z

pkg/client/client_test.go

+func (h *httpClient) RoundTrip(req *http.Request) (*http.Response, error) {
+	h.r = req
+	return h.resp, nil
 }


Do we even need RoundTrip implementation then?

Yes, since we to pass it to client.NewBasicAuthRoundTripper in the TestBasicAuth.

Otherwise it fails with:

# github.com/flatcar-linux/fleetlock/pkg/client_test [github.com/flatcar-linux/fleetlock/pkg/client.test] ./client_test.go:159:45: cannot use tr (type *httpClient) as type http.RoundTripper in argument to client.NewBasicAuthRoundTripper: *httpClient does not implement http.RoundTripper (missing RoundTrip method)

But with the comment below, we could just pass nil here and use the default, so we don't need to implement this 😄

That's true - but we lose the ability to actually assert that the request has been made using Basic Authentication since we have the following:

tr := &httpClient{ resp: &http.Response{ StatusCode: 200, }, } ... u, p, ok := tr.r.BasicAuth() if u != username || p != password || !ok { t.Fatalf("basic auth creds do not match") }

Same, you can inspect received request in your mock client, no?

I would wished.

In this test, we directly use net/http.Client to be able to pass the Transport: client.NewBasicAuthRoundTripper(username, password, tr) - we don't use the httpClient mock so we are not able to inspect the recorded request. 🤔

Might be an overkill, but worst case we could spawn a test server and test requests there.

I just thought that if we go this way (passing a nil transport to use the default one):

client.NewBasicAuthRoundTripper(username, password, nil)

we'll do an actual HTTP call and lose the mock ability which is currently done via :

tr := &httpClient{ resp: &http.Response{ StatusCode: 200, }, }

I think mocking via Do() method, as changed also in #3 is more effective in this case.

invidian · 2021-10-28T13:22:22Z

pkg/client/authentication.go

+}
+
+// NewBasicAuthRoundTripper returns a basicAuthRoundTripper with username and password.
+func NewBasicAuthRoundTripper(username, password string, rt http.RoundTripper) http.RoundTripper {


Maybe we can let rt to be nil and use net/http.Transport{} as default?

invidian · 2021-10-28T13:23:42Z

pkg/client/client.go

+		id, err := machineID()
+		if err != nil {
+			return nil, fmt.Errorf("getting machine ID: %w", err)
+		}


This seems like something CLI code should do, as it cannot be easily tested.

We could using afero.FS to abstract file system access - but I feel it's a bit overkill for this, let's move it to the CLI section.

Now, I'm thinking from a lib point of view. It would mean that ID is actually required to construct a fleetlock.Client, the default value is machineID() but if we move this section to the CLI it makes it mandatory 🤔

Yeah, I think that make sense. If we have more than one required value, maybe we can move them all to the config struct then? :D

invidian · 2021-10-28T13:24:28Z

pkg/client/client.go

+	fleetlock.group = cfg.Group
+	fleetlock.id = cfg.ID
+	fleetlock.http = cfg.HTTP
+
+	if fleetlock.group == "" {
+		fleetlock.group = "default"
+	}


Do we have tests for this code?

Not yet, but this fields are not exported - not sure how we can test them since the test file is in its own package client_test 🤔 by adding some getters ?

func (c *client) Group() string { return c.group } ...

You should be able to receive the sent request and inspect it I think, no?

Done - we add two new parameters to the test case definition: cfg and expCfg.
The first one is used to build the client.New(test.cfg) and the second is used to assert that we effectively have the right parameters after inspecting the request content. :)

tormath1 · 2021-11-02T12:19:49Z

@invidian what about rebasing onto main to include #3 changes ? If it's ok with you, I'll squash / reword commits too to have a better view now.

invidian · 2021-11-02T13:07:49Z

@invidian what about rebasing onto main to include #3 changes ? If it's ok with you, I'll squash / reword commits too to have a better view now.

Sure. I don't care about the git history of the workflow, as IMO this is transient, I'm much more interested into getting clean history merged into the main branch.

I think rebasing is fine. I'm still thinking how we can test those things better, so I may propose something, but I think it shouldn't be such a change like from #3.

This struct can be used to pass parameters to create the FleetLock client. Signed-off-by: Mathieu Tortuyaux <[email protected]>

`client.Config` can be passed to FleetLock client when we create it. Signed-off-by: Mathieu Tortuyaux <[email protected]>

this values are now optionals. Default values can be overrided with config. ID and URL are mandatory. Signed-off-by: Mathieu Tortuyaux <[email protected]>

we test mandatory config parameters too. Signed-off-by: Mathieu Tortuyaux <[email protected]>

Signed-off-by: Mathieu Tortuyaux <[email protected]>

This round tripper can be used to do a correct basic authentication Signed-off-by: Mathieu Tortuyaux <[email protected]>

Signed-off-by: Mathieu Tortuyaux <[email protected]>

tormath1 · 2021-11-02T17:13:28Z

@invidian rebased on main :) - just need to address the lint issues

invidian · 2021-11-03T11:37:21Z

https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis is a good article describing why option functions might be better over the configuration struct.

tormath1 · 2021-11-03T14:36:10Z

https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis is a good article describing why option functions might be better over the configuration struct.

I personally find this approach more prone to scale since it abstracts the configuration of the struct. That's why I initially started with this way - but as you mentioned since it's logicless, using config might be more relevant

invidian · 2021-11-04T08:29:32Z

If we can wait with this couple of days more, I'm working on getting some examples what kind of tests we could roll out to CLI code, if we care about this.

tormath1 · 2021-11-04T08:57:22Z

@invidian

[...] what kind of tests we could roll out to CLI code, if we care about this.

CLI is currently just here to provide a simple example of FleetLock implementation - as it's currently designed to be first used as a lib (with Locksmith for example with this PoC: flatcar/locksmith#14). :)

If we can wait with this couple of days more

Yeah definitely.

invidian

After looking at the code again, I've realized that this PR is likely doing too many things at a time, which breaks down the focus on individual parts, so getting the quality bar high on all of them will take bunch of rounds of reviews.

I'd like to suggest, that we break this PR down into individual pieces, to make the change easy (refactoring), so then we make easy change (add basic auth support).

I see the following independent threads here:

Testing payload which client sends.
Converting parameters to config struct.
Reading machine-id from file.
Adding basic auth round tripper.
Validating constructor parameters.

On the other hand, I don't want to block this feature, so I'm fine with merging it as it is, as the PR is correct, just some things could be improved.

We can also not touch some things (like payload testing) and just focus on basic auth functionality.

Regarding testing the CLI code, I've created this repository: https://github.com/invidian/golang-cli-testing-example, which based on my knowledge right now has good examples on how to split and test the CLI code in a manageable way. The examples are a little bit extreme in some cases, but I wanted to give a variety of them, so also not so common scenarios are covered (like for example isolating and testing stdout).

invidian · 2021-11-15T08:17:05Z

cmd/root.go

+// machineID is a helper to return unique ID
+// of the machine.
+func machineID() (*string, error) {
+	id, err := ioutil.ReadFile("/etc/machine-id")


Making this path configurable would make the more testable, but I guess it would have to be done via e.g. --id-from-file flag or something. Also, we don't have any tests in place for this, so would be more effort.

I get your point, however we should not forget that machineID() is a helper to provide a default value for the --id flag. In case one wants to use a different ID he can just use --id my-id or even --id $(cat /tmp/my-id-file).

If we really want to test this helper, we can still rely on fs abstraction with afero.FS but it becomes a bit overkill IMHO. :)

invidian · 2021-11-15T08:17:41Z

cmd/unlock.go

+			if id == nil {
+				var err error
+				id, err = machineID()
+				if err != nil {
+					return fmt.Errorf("getting machine ID: %w", err)
+				}
+			}


This code is now duplicate. Can we refactor?

invidian · 2021-11-15T08:19:03Z

pkg/client/authentication.go

+}
+
+// NewBasicAuthRoundTripper returns a basicAuthRoundTripper with username and password.
+func NewBasicAuthRoundTripper(username, password string, rt http.RoundTripper) http.RoundTripper {


Looks like round tripper should have it's own set of tests, separate from the client, to make the effort required for testing it smaller.

invidian · 2021-11-15T08:20:11Z

pkg/client/client.go

+
+// New builds a FleetLock client.


Why do we move this function to the bottom of the file? I think right after exported struct definition it's very good, as this what potential reader will be looking for initially.

I agree - It might be related to a rebase onto main.

invidian · 2021-11-15T09:28:19Z

pkg/client/client_test.go

+			if _, _, ok := h.r.BasicAuth(); ok {
+				t.Fatalf("basic auth should not be set")
+			}


I don't think this should be part of this test. We only need one test I think, verifying that client use given round tripper, which can as well be our test one.

invidian · 2021-11-15T09:29:25Z

pkg/client/client_test.go

+			payload := getPayload(h)
+
+			if payload.ClientParams.Group != test.expCfg.Group {
+				t.Fatalf("payload's group should be : %s, got: %s", test.expCfg.Group, payload.ClientParams.Group)
+			}


I like checking the payload. However, if we do that, should we also verify the headers?

To assert this: https://github.com/flatcar-linux/fleetlock/blob/main/pkg/client/client.go#L117 ?

https://github.com/flatcar-linux/fleetlock/blob/8937d308dfa43a7f4ec308005c9b75c569e0e79a/pkg/client/client.go#L117-L118 to be precise, yes 👍

Signed-off-by: Mathieu Tortuyaux <[email protected]>

invidian · 2021-11-15T19:26:42Z

cmd/root.go

+}
+
+// checkID asserts that the ID is not nil, if it's the case
+// it uses `machineID` to generate a default one.


We do not generate anything.

Suggested change

// it uses `machineID` to generate a default one.

// it uses `machineID` to set a default one.

invidian · 2021-11-15T19:29:16Z

cmd/root.go

+
+// checkID asserts that the ID is not nil, if it's the case
+// it uses `machineID` to generate a default one.
+func checkID(id *string) error {


I don't think this function should have check in the name and only return error. Right now it strangely looks like a validation function, which is not really the case, as it may actually mutate the given id.

Maybe we can make it return (string, err) and rename to e.g. getID?

invidian · 2021-11-15T22:47:36Z

pkg/client/authentication.go

+		return resp, nil
+	}
+
+	req = req.Clone(context.TODO())


Suggested change

req = req.Clone(context.TODO())

req = req.Clone(req.Context())

Otherwise using round tripper swallows the context control on the request:

package client_test import ( "context" "errors" "net/http" "testing" "time" "github.com/flatcar-linux/fleetlock/pkg/client" ) func Test_Cancelling_context_for_request_performed_with_http_client_with_basic_auth_round_tripper_cancels_the_request(t *testing.T) { httpClient := http.Client{ Transport: client.NewBasicAuthRoundTripper("foo", "bar", nil), } requestTimeout := time.Second ctx, cancel := context.WithTimeout(context.Background(), requestTimeout) t.Cleanup(cancel) req, err := http.NewRequestWithContext(ctx, "GET", "http://10.255.255.1", nil) if err != nil { t.Fatal(err) } errCh := make(chan error, 1) go func() { _, err = httpClient.Do(req) errCh <- err }() testDeadline := time.NewTimer(2 * requestTimeout) select { case <-testDeadline.C: t.Fatalf("Expected request to return before the deadline") case err := <-errCh: if err != nil && !errors.Is(err, context.DeadlineExceeded) { t.Fatal(err) } } }

tormath1 requested a review from invidian October 27, 2021 07:45

tormath1 self-assigned this Oct 27, 2021

invidian requested changes Oct 27, 2021

View reviewed changes

pkg/client/options.go Outdated Show resolved Hide resolved

pkg/client/options.go Outdated Show resolved Hide resolved

pkg/client/client.go Outdated Show resolved Hide resolved

pkg/client/client.go Outdated Show resolved Hide resolved

tormath1 force-pushed the tormath1/basic-auth branch from 5848a0c to 25fecc7 Compare October 27, 2021 12:27

invidian reviewed Oct 27, 2021

View reviewed changes

tormath1 mentioned this pull request Oct 27, 2021

client: remove HTTPClient interface #2

Closed

invidian reviewed Oct 28, 2021

View reviewed changes

pkg/client/client_test.go Outdated Show resolved Hide resolved

pkg/client/options.go Outdated Show resolved Hide resolved

invidian reviewed Oct 28, 2021

View reviewed changes

tormath1 mentioned this pull request Oct 29, 2021

[WIP] Integrate Fleetlock client with Locksmith flatcar/locksmith#14

Open

tormath1 added 7 commits November 2, 2021 17:49

pkg/client: add Config struct

25ce529

This struct can be used to pass parameters to create the FleetLock client. Signed-off-by: Mathieu Tortuyaux <[email protected]>

client: use config to create Client

6b9138f

`client.Config` can be passed to FleetLock client when we create it. Signed-off-by: Mathieu Tortuyaux <[email protected]>

pkg/client: add default for group and client

a65477b

this values are now optionals. Default values can be overrided with config. ID and URL are mandatory. Signed-off-by: Mathieu Tortuyaux <[email protected]>

pkg/client/test: update test to use config

725a17c

we test mandatory config parameters too. Signed-off-by: Mathieu Tortuyaux <[email protected]>

cmd: get default ID from /etc/machine-id

131fbe0

Signed-off-by: Mathieu Tortuyaux <[email protected]>

pkg/client: add basic auth round tripper

e6cfe16

This round tripper can be used to do a correct basic authentication Signed-off-by: Mathieu Tortuyaux <[email protected]>

pkg/client/test: add basic authentication test

b0096da

Signed-off-by: Mathieu Tortuyaux <[email protected]>

tormath1 force-pushed the tormath1/basic-auth branch from 65febb2 to b0096da Compare November 2, 2021 17:09

invidian reviewed Nov 15, 2021

View reviewed changes

tormath1 added 2 commits November 15, 2021 15:43

fixup! pkg/client/test: add basic authentication test

0a91d9f

pkg/client/test: test fleetlock header

041a9fb

Signed-off-by: Mathieu Tortuyaux <[email protected]>

tormath1 added 2 commits November 15, 2021 15:46

fixup! client: use config to create Client

e10d5fc

fixup! cmd: get default ID from /etc/machine-id

03555bc

invidian reviewed Nov 15, 2021

View reviewed changes

	// it uses `machineID` to generate a default one.
	// it uses `machineID` to set a default one.

	req = req.Clone(context.TODO())
	req = req.Clone(req.Context())

client: add basic auth support #1

Are you sure you want to change the base?

client: add basic auth support #1

Conversation

tormath1 commented Oct 27, 2021 • edited Loading

Testing done

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

tormath1 commented Oct 27, 2021

tormath1 commented Oct 28, 2021 • edited Loading

tormath1 commented Oct 28, 2021

invidian 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

tormath1 Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

tormath1 commented Nov 2, 2021

invidian commented Nov 2, 2021

tormath1 commented Nov 2, 2021 • edited Loading

invidian commented Nov 3, 2021

tormath1 commented Nov 3, 2021

invidian commented Nov 4, 2021

tormath1 commented Nov 4, 2021 • edited Loading

invidian 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

tormath1 commented Oct 27, 2021 •

edited

Loading

tormath1 commented Oct 28, 2021 •

edited

Loading

tormath1 Oct 29, 2021 •

edited

Loading

tormath1 commented Nov 2, 2021 •

edited

Loading

tormath1 commented Nov 4, 2021 •

edited

Loading