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

use fqdn for module #74

Merged
merged 4 commits into from
Dec 20, 2023
Merged

Conversation

drewwells
Copy link
Contributor

Used gofmt to rename ssh3 to github.com/francoismichel/ssh3. The code hasn't been formatted so this resulted in a lot of whitespace changes.

@mpiraux
Copy link
Collaborator

mpiraux commented Dec 20, 2023

Before I jump into the changes, it would be nice to find a way to enforce gofmt. Ideally it would be in a separated workflow. But also being able to run it before commit would be a good thing.

Maybe making a Makefile ? It could also setup a pre-commit script, but I'm not very knowledgeable on this part.
This could end up in a dedicated issue though.

Copy link
Collaborator

@mpiraux mpiraux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I eyeballed all the changes and they are fine

.github/workflows/build.yml Show resolved Hide resolved
@drewwells
Copy link
Contributor Author

Before I jump into the changes, it would be nice to find a way to enforce gofmt. Ideally it would be in a separated workflow. But also being able to run it before commit would be a good thing.

Maybe making a Makefile ? It could also setup a pre-commit script, but I'm not very knowledgeable on this part. This could end up in a dedicated issue though.

I agree, the workflow should be turned into a makefile.

as far as enforcing go fmt, the code I add would do this. Most people configure their ide to use go fmt.

Copy link
Owner

@francoismichel francoismichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the formatting, I wish I did that before releasing the implem publicly.

A few important requested changes needed to keep interoperability though. I can commit it myself if you want.

@@ -648,7 +642,7 @@ func mainWithStatusCode() int {
if err != nil {
log.Fatal().Msgf("%s", err)
}
req.Proto = "ssh3"
req.Proto = "github.com/francoismichel/ssh3"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that one, though, we're setting the :protocol HTTP/3 pseudo-header here like it is done fir webtransport, or masque. So I would let that set to ssh3 for now.

@@ -53,7 +54,7 @@ func (i *PubKeyIdentity) Verify(genericCandidate interface{}, base64Conversation
return nil, fmt.Errorf("unsupported signature algorithm '%s' for %T", unvalidatedToken.Method.Alg(), i)
},
jwt.WithIssuer(i.username),
jwt.WithSubject("ssh3"),
jwt.WithSubject("github.com/francoismichel/ssh3"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as my above comment, if another implem appears, we would not we interoperable, so I would keep ssh3 for now

server.go Outdated
@@ -130,7 +130,7 @@ func (s *Server) GetHTTPHandlerFunc(ctx context.Context) AuthenticatedHandlerFun

return func(authenticatedUsername string, newConv *Conversation, w http.ResponseWriter, r *http.Request) {
log.Info().Msgf("got request: method: %s, URL: %s", r.Method, r.URL.String())
if r.Method == http.MethodConnect && r.Proto == "ssh3" {
if r.Method == http.MethodConnect && r.Proto == "github.com/francoismichel/ssh3" {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I would keep ssh3 to keep interoperability

client.go Outdated Show resolved Hide resolved
Copy link
Owner

@francoismichel francoismichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed every change, looks good to me. Thanks for the reformat !

@francoismichel francoismichel merged commit c39bb79 into francoismichel:main Dec 20, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants