Add basic support & documentation for built-in and plugin-based auth #51

vdye · 2023-05-06T00:31:47Z

Closes #41

This pull request lays the foundation for specialized auth configuration in the bundle server:

Commit 1 creates the AuthResult type, which the web server will use to relay the output of a custom auth function as an HTTP response
Commit 2 adds the --auth-config type and initial schema; in this commit, it doesn't do much of anything (since there aren't any valid modes yet). Also introduce the AuthMiddleware interface, which will be used in both built-in modes and the basis for plugin modes.
Commit 3 upgrades the version of Go used by the bundle server to 1.20 (necessary for Commit 4)
Commit 4 adds the fixed built-in auth mode, which validates a request's Basic header against a static username/password pair.
Commits 5 & 6 update the integration tests to include tests of the fixed auth mode
Commit 7 adds the plugin auth mode, which allows a user to load (at runtime) a custom implementation of AuthMiddleware

This is a pretty major feature (or the start of one, at least), so I'd really appreciate an extra reviewer focus on making sure the architecture is sound and there aren't any glaring security issues (other than what's already mentioned in the docs, i.e. "don't run untrusted plugins").

derrickstolee

Neat plugin interface. I had some comments that might help as you harden things.

derrickstolee · 2023-05-08T13:36:55Z

cmd/git-bundle-web-server/bundle-server.go

+		if authResult.WriteResponse(w) {
+			return
+		}


This is a critical path to understand, so it might be worth a comment.

// Write a 401 or 404, if needed if authResult.WriteResponse(w) { return } // Authorized, continue

The other way to think about it is to consider "authorized" as a boolean condition, and if false there is something to be done with WriteResponse:

if b.authorize != nil { authResult := b.authorize(r, owner, repo) if !authResult.authorized() { authResult.WriteResponse(w) return } }

I originally had it as the latter, but I wasn't thrilled with needing to re-check Authorized() in WriteResponse() (because if I was doing that anyway, why even have the original Authorized() check?).

I'll add a comment, but I'll also try to come up with a better name (AuthorizeOrWriteResponse()? or maybe something simpler like ApplyResult()).

If we wanted to give auth middleware full control of the pipeline then really we should really only expect a boolean "return bundles or exit?" and have the middleware handle the entire response?

if b.authorize != nil { if !b.authorize(r, w, owner, repo) { return } }

I assume the only expected front-end/consumer of the bundle server endpoints is Git (never a browser etc)?

It took a few iterations, but I've made the AuthResults a bit more generic (where Allow() == "apply headers & serve content" and Deny() == "apply headers & respond with given 4XX code"). I'd like to keep the middleware at least a bit sandboxed (it helps keep the bundle server aligned with what Git expects/supports and IMO makes plugin writing simpler), but if there's anything else that might be valuable to add to the current interface, I'm happy to update!

cmd/git-bundle-web-server/main.go

docs/technical/auth-config.md

cmd/git-bundle-web-server/main.go

docs/technical/auth-config.md

pkg/auth/middleware.go

derrickstolee · 2023-05-08T13:58:17Z

cmd/git-bundle-web-server/main.go

+		}
+
+		// First, verify plugin checksum matches expected
+		// NEEDSWORK: time-of-check/time-of-use


Just pointing out the NEEDSWORK here, probably part of your "draft" status?

But what else can we do? Can we access the file descriptor of the plugin from p.Open() to verify the hash of exactly what we opened? Does the plugin library have security checks?

The plugin library has no way of doing that, unfortunately 😞. I considered opening an issue in golang/go, but the more I thought about it the less feasible that seemed. Under the hood, Go is using dlopen & dlsym to load the plugin symbols, so even if it did verify the checksum before running the init tasks of the Go package, someone could still (carefully) overwrite the file contents before invoking the initializer symbol.

If we really wanted to do this, we'd probably need to write our own version of plugin that:

opens the file with dlopen()

locks it with flock()

checks the checksum

loads & runs the init tasks

loads the requested symbol with dlsym()

invokes the symbol

releases the lock with flock()

closes the file

Nearly all of that would need to be done with C bindings and would need platform-specific implementations for Mac & Linux due to slight differences in flock() (and would further distance us from Windows support, if that matters?). To be honest, that doesn't necessarily seem worth it.

So my next question is, should we remove/change the corresponding warning in docs/technical/auth-config.md to drop the specific mention of TOCTOU and just recommend general safety around custom plugins?

If no one's opposed, I'll leave the TOCTOU mention - seems better to inform users to watch for/mitigate it than to pretend it's not an issue.

docs/technical/auth-config.md

mjcheetham

One overarching question I had is do we want to relax the plugin interface even more? From the bundle server's perspective the only real decision it needs to make is "should I return bundles or not".

Splitting out unauthorized 401, forbidden 403(404?) and success (200) means plugins can only use those response options.

pkg/auth/auth-result.go

docs/technical/auth-config.md

Add an 'authorize' function to the bundle web server for managing access to bundle server content. This function returns one of two states: - Deny, which indicates that bundle server content should not be served. This is configured with the desired 4XX response code and (optional) headers. Invoking 'ApplyResult()' with this result will fill in the response information and trigger 'serve' to exit immediately. - Allow, indicating that the requstor may access the requested resource. This does not return an immediate response, but if headers are specified, they will be added to the eventual response. After applying this result with 'ApplyResult()', 'serve' will continue on to add the appropriate content to the response. For now, the 'authorize' function is always nil, so it's never invoked. In later patches, it will be configured via a new '--auth-config' option to the web server. Signed-off-by: Victoria Dye <[email protected]>

Add an '--auth-config' flag to specify the path to a JSON file containing information used to configure bundle server auth, as well as a function for parsing the config. For now, no modes are configured, so attempting to pass a value to the option will fail. Update the 'git-bundle-web-server' manpage to describe the auth config and its usage. Also add 'auth-config.md' technical documentation with more detail around the schema of the auth config file. These will be updated as new modes are introduced. Signed-off-by: Victoria Dye <[email protected]>

Upgrade the minimum Go version for the project. This update is necessary to take advantage of some useful new features (including, in an upcoming patch, the ability to convert between variable- and fixed-length byte slices). Signed-off-by: Victoria Dye <[email protected]>

vdye · 2023-05-12T00:03:22Z

@derrickstolee @mjcheetham I think I've addressed all of the feedback so far. The PR has changed a fair amount since the first round of reviews (added integration tests, manpage docs, and better in-code documentation for pkg/auth; also, changed AuthResult pretty substantially), so please let me know if you have any questions or otherwise think something should be changed! 🙇‍♀️

ldennington

Looks really good! Very impressed all-up, but the plugin work is especially awesome.

examples/auth/config/fixed.json

internal/auth/middleware.go

ldennington · 2023-05-15T17:52:04Z

internal/auth/middleware.go

+
+	// Check for invalid username characters
+	if strings.Contains(params.Username, ":") {
+		return nil, fmt.Errorf("username contains a colon (\":\")")


Why can't usernames contain colons specifically?

I'd imagine to avoid issues encoding this in a URL?

protocol://user:pass@host/path

Same with @; you'd need to escape it.

It's part of RFC 7617, which defines Basic auth:

Furthermore, a user-id containing a colon character is invalid, as the first colon in a user-pass string separates user-id and password from one another; text after the first colon is part of the password. User-ids containing colons cannot be encoded in user-pass strings.

ldennington · 2023-05-15T18:00:21Z

internal/auth/middleware_test.go

+	expectedResponseCode    int
+	expectedHeaders         http.Header
+}{
+	{


Really thorough set of test cases here!

docs/technical/auth-config.md

examples/auth/config/plugin.json

Create and test a built-in auth mode for a single fixed username/password authenticated against a provided 'Authorization: Basic' header, implementing the AuthMiddleware interface. Add the mode to 'parseAuthConfig()' in 'git-bundle-web-server' to enable its usage in the web server. Update documentation for the mode in 'docs/technical/auth-config.md' and 'git-bundle-web-server' manpage, and an explicit example JSON in the 'examples/auth/config' directory. Signed-off-by: Victoria Dye <[email protected]>

Update the 'bundleServer.startWebServer()' method to wait for indication that the web server has successfully started (the "Server is running at address..." message) before returning successfully. If the process stops before that, or a timeout of 2s is reached, an error is thrown. However, because the "Server is running..." printout happens in parallel with the setup done in 'ListenAndServe[TLS]()', it is possible that the message is printed and immediately followed with an exit due to a server startup error. To mitigate that, add a 0.1s pause before printing the message and, in the tests, only set the status to 'ok' if the 'close' event has not been triggered by the time the startup message is processed. Signed-off-by: Victoria Dye <[email protected]>

Add tests of the bundle web server with no auth and with a 'fixed' auth configuration. Exercise cases where the user provides no credentials, incorrect credentials, and correct credentials and the expected bundle server responses for each. Signed-off-by: Victoria Dye <[email protected]>

Implement and document auth configurations using user-specified runtime plugins. The goal of this change is to allow users to customize their access control to a more specific application or domain than is supported by built-in modes. A plugin is loaded in 'git-bundle-web-server' from its 'path' by first comparing the file contents to a specified SHA256 checksum; the web server fails to start if there is a mismatch. Otherwise, the plugin is loaded and the specified 'initializer' symbol is looked up. If that symbol exists, the initializer is called, creating the 'AuthMiddleware' instance and/or an error. From there, 'git-bundle-web-server' passes the middleware's 'Authorize' function reference to the created 'bundleWebServer' so that it is invoked after parsing the route of each request. Additionally, update manpage and technical documentation. Add an example plugin (built from a standalone '.go' file) & config to 'examples/auth/'. Signed-off-by: Victoria Dye <[email protected]>

vdye added this to the v1.0 milestone May 6, 2023

vdye requested a review from mjcheetham May 6, 2023 00:31

vdye self-assigned this May 6, 2023

vdye requested review from derrickstolee and ldennington as code owners May 6, 2023 00:31

vdye marked this pull request as draft May 6, 2023 00:32

derrickstolee reviewed May 8, 2023

View reviewed changes

vdye commented May 8, 2023

View reviewed changes

docs/technical/auth-config.md Outdated Show resolved Hide resolved

mjcheetham reviewed May 10, 2023

View reviewed changes

pkg/auth/auth-result.go Outdated Show resolved Hide resolved

pkg/auth/auth-result.go Outdated Show resolved Hide resolved

docs/technical/auth-config.md Outdated Show resolved Hide resolved

vdye force-pushed the vdye/auth-plugins branch 2 times, most recently from 490d7e8 to 065d28d Compare May 10, 2023 23:37

vdye marked this pull request as ready for review May 10, 2023 23:39

vdye force-pushed the vdye/auth-plugins branch from 065d28d to 40a0248 Compare May 11, 2023 00:17

vdye added 3 commits May 11, 2023 14:39

vdye force-pushed the vdye/auth-plugins branch from 40a0248 to ec13561 Compare May 11, 2023 21:49

vdye mentioned this pull request May 11, 2023

Add a git-bundle-server command or installed script for generating auth configs #52

Open

ldennington approved these changes May 15, 2023

View reviewed changes

vdye added 4 commits May 16, 2023 09:37

vdye force-pushed the vdye/auth-plugins branch from ec13561 to fdb0d44 Compare May 16, 2023 16:37

vdye merged commit 35a95e4 into git-ecosystem:main May 17, 2023

vdye deleted the vdye/auth-plugins branch May 17, 2023 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support & documentation for built-in and plugin-based auth #51

Add basic support & documentation for built-in and plugin-based auth #51

vdye commented May 6, 2023 •

edited

Loading

derrickstolee left a comment

derrickstolee May 8, 2023

vdye May 8, 2023

mjcheetham May 10, 2023

vdye May 11, 2023

derrickstolee May 8, 2023

derrickstolee May 8, 2023

vdye May 9, 2023

vdye May 11, 2023

mjcheetham left a comment

vdye commented May 12, 2023

ldennington left a comment

ldennington May 15, 2023

mjcheetham May 15, 2023

vdye May 16, 2023

ldennington May 15, 2023

Add basic support & documentation for built-in and plugin-based auth #51

Add basic support & documentation for built-in and plugin-based auth #51

Conversation

vdye commented May 6, 2023 • edited Loading

derrickstolee 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

mjcheetham left a comment

Choose a reason for hiding this comment

vdye commented May 12, 2023

ldennington 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

vdye commented May 6, 2023 •

edited

Loading