Prep for OSS #3
Merged
Prep for OSS #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stage the code here assuming that we're going to move it to the Connect Github organization.
Amend the Makefile to automatically (re-)generate code as part of the build.
Clarify some GoDoc, removing a reference to the now-deleted Interceptor type.
We don't use these GHA in the Connect project.
As much as possible, focus the GoDoc examples on the actual authentication logic: lead with the AuthFunc, and factor as much of the TLS setup as possible into helper functions. Eventually, it would be nice to use memhttp here - especially if we bake in helpers for mTLS.
Add accessors for the standard library's cookie methods. IMO, there's not enough here to warrant testing - we'd just be exercising net/http.
In the README, make the AuthFunc even more prominent. Also, reformat to two spaces instead of tabs (so Github's UI is less likely to scroll horizontally).
emcfarlane
reviewed
Dec 12, 2023
authn.go
Outdated
| // wrapped handler without authentication. | ||
| // arbitrary information about the authenticated identity to the context. Any | ||
| // non-RPC requests (as determined by their Content-Type) are forwarded | ||
| // directly to the wrapped handler without authentication. |
There was a problem hiding this comment.
We should maybe revisit this. It might be surprising to users that authentication is not invoked on non RPC methods and could be an attack vector if users can devise a way to fool the classifier for the authenticator but for connect-go to still accept it in the handler.
There was a problem hiding this comment.
Actually I think this will fail for all GET requests as Content-Type doesn't have to be present 😟
There was a problem hiding this comment.
Hm, that's a good point 😮 I'll change this!
There was a problem hiding this comment.
Ill fix up errorWriter to classify gets if the header or url string is present.
There was a problem hiding this comment.
Co-authored-by: Edward McFarlane <[email protected]>
emcfarlane
reviewed
Dec 12, 2023
Even in examples, use `crypto/subtle` to minimize attack surface. Along the way, fix the places where I confused the Aladdin and Ali Baba stories.
emcfarlane
approved these changes
Dec 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR prepares this repository for a move to the Connect Github organization
and a public release. Most of the commits are cleanups and preparation for the
move, but a few were more substantive (most importantly, we weren't generating
code as part of the build process).
Best reviewed commit-by-commit.