Skip to content

[FEAT] Add server_name field in ClientTLS auth request claims #210

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[FEAT] Add server_name field in ClientTLS auth request claims #210

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions v2/authorization_claims.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ type ClientTLS struct {
Cipher string `json:"cipher,omitempty"`
Certs StringList `json:"certs,omitempty"`
VerifiedChains []StringList `json:"verified_chains,omitempty"`
ServerName string `json:"server_name,omitempty"`
Copy link
Member

@derekcollison derekcollison Oct 25, 2023

Choose a reason for hiding this comment

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

Should this be HostName? Would that be more descriptive?

Copy link
Author

@charbonnierg charbonnierg Oct 25, 2023

Choose a reason for hiding this comment

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

I used the same terminology as crypto/tls: https://pkg.go.dev/crypto/tls#ConnectionState, and I though that in the context of a ClientTLS block, ServerName would have a clear meaning.

But I understand that it may be misleading due to the fact that a server also has a server_name, so I would like for you to make the choice 😅

Copy link
Member

@derekcollison derekcollison Oct 25, 2023

Choose a reason for hiding this comment

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

Good point, we can stick with ServerName or SNI.

Copy link
Author

@charbonnierg charbonnierg Oct 25, 2023

Choose a reason for hiding this comment

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

I think that SNI may be the best candidate here. Googling "tls sni" gives pretty relevant results, and it does not reuse an existing field name from other claims data.

If that's ok with you, I can push a commit with this modification.

Copy link
Author

@charbonnierg charbonnierg Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The change would be:

	VerifiedChains []StringList `json:"verified_chains,omitempty"`
-       ServerName   string        `json:"server_name,omitempty"`
+       SNI            string      `json:"sni,omitempty"`

Copy link
Member

@derekcollison derekcollison Oct 25, 2023

Choose a reason for hiding this comment

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

Looping in @philpennock and @ripienaar for their opinions here. I could do either.

}

// AuthorizationRequest represents all the information we know about the client that
Expand Down