Skip to content

Commit

Permalink
Improve handling of public and confidential clients
Browse files Browse the repository at this point in the history
  • Loading branch information
giftkugel committed Aug 28, 2024
1 parent acf49dd commit be875ad
Show file tree
Hide file tree
Showing 9 changed files with 111 additions and 45 deletions.
13 changes: 8 additions & 5 deletions config.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
server:
#logLevel: error
# logLevel: debug
cookies:
authName: stopnik_auth
messageName: stopnik_message
Expand All @@ -24,12 +24,12 @@ clients:
accessTTL: 5
refreshTTL: 15
idTTL: 15
type: confidential
oidc: true
introspect: true
redirects:
- https://oauth.pstmn.io/v1/callback
- http://localhost:8080/session/callback
- http://localhost:5173/reporting/oidc-callback*
claims:
- name: foo
value: bar
Expand All @@ -39,15 +39,13 @@ clients:
accessTTL: 5
refreshTTL: 15
opaqueToken: true
type: confidential
redirects:
- https://oauth.pstmn.io/v1/callback
- id: testclient3
clientSecret: 1efcbc37f7d7e2f9f8cf009b91c95b2b7b913b89d36a21a05da1e3cb396ed1ab0e596e2b649e9407367e40d852ac4d0abfcfc1c4227eb661385e9f2e0f3203ca
salt: 321
accessTTL: 5
refreshTTL: 15
type: confidential
privateKey: ./test_keys/ecdsa521key.pem
redirects:
- https://oauth.pstmn.io/v1/callback
Expand All @@ -57,4 +55,9 @@ users:
salt: moo
profile:
givenName: John
familyName: Doe
familyName: Doe
address:
street: Mainstreet 1
city: Sometown
postalCode: 12345
country: Boom
88 changes: 61 additions & 27 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import (
"errors"
"fmt"
internalHttp "github.com/webishdev/stopnik/internal/http"
"github.com/webishdev/stopnik/internal/oauth2"
"github.com/webishdev/stopnik/log"
"math/big"
"strings"
)

type Keys struct {
Expand Down Expand Up @@ -38,39 +40,41 @@ type Server struct {
}

type UserAddress struct {
Street string `yaml:"street"`
City string `yaml:"city"`
PostalCode string `yaml:"postalCode"`
Region string `yaml:"region"`
Country string `yaml:"country"`
Formatted string `json:"formatted,omitempty"`
Street string `yaml:"street" json:"street_address,omitempty"`
City string `yaml:"city" json:"locality,omitempty"`
PostalCode string `yaml:"postalCode" json:"postal_code,omitempty"`
Region string `yaml:"region" json:"region,omitempty"`
Country string `yaml:"country" json:"country,omitempty"`
}

type UserProfile struct {
Subject string `json:"sub,omitempty"`
Name string `json:"name,omitempty"`
GivenName string `yaml:"givenName" json:"given_name,omitempty"`
FamilyName string `yaml:"familyName" json:"family_name,omitempty"`
Nickname string `yaml:"nickname" json:"nickname,omitempty"`
PreferredUserName string `yaml:"preferredUserName" json:"preferred_username,omitempty"`
Email string `yaml:"email" json:"email,omitempty"`
EmailVerified bool `yaml:"emailVerified" json:"email_verified,omitempty"`
Gender string `yaml:"gender" json:"gender,omitempty"`
BirthDate string `yaml:"birthDate" json:"birth_date,omitempty"`
ZoneInfo string `yaml:"zoneInfo" json:"zone_info,omitempty"`
Locale string `yaml:"locale" json:"locale,omitempty"`
PhoneNumber string `yaml:"phoneNumber" json:"phone_number,omitempty"`
PhoneVerified bool `yaml:"phoneVerified" json:"phone_verified,omitempty"`
Website string `yaml:"website" json:"website,omitempty"`
Profile string `yaml:"profile" json:"profile,omitempty"`
ProfilePicture string `yaml:"profilePicture" json:"profile_picture,omitempty"`
Subject string `json:"sub,omitempty"`
Name string `json:"name,omitempty"`
GivenName string `yaml:"givenName" json:"given_name,omitempty"`
FamilyName string `yaml:"familyName" json:"family_name,omitempty"`
Nickname string `yaml:"nickname" json:"nickname,omitempty"`
PreferredUserName string `yaml:"preferredUserName" json:"preferred_username,omitempty"`
Email string `yaml:"email" json:"email,omitempty"`
EmailVerified bool `yaml:"emailVerified" json:"email_verified,omitempty"`
Gender string `yaml:"gender" json:"gender,omitempty"`
BirthDate string `yaml:"birthDate" json:"birth_date,omitempty"`
ZoneInfo string `yaml:"zoneInfo" json:"zone_info,omitempty"`
Locale string `yaml:"locale" json:"locale,omitempty"`
PhoneNumber string `yaml:"phoneNumber" json:"phone_number,omitempty"`
PhoneVerified bool `yaml:"phoneVerified" json:"phone_verified,omitempty"`
Website string `yaml:"website" json:"website,omitempty"`
Profile string `yaml:"profile" json:"profile,omitempty"`
ProfilePicture string `yaml:"profilePicture" json:"profile_picture,omitempty"`
Address UserAddress `yaml:"address" json:"address,omitempty"`
UpdatedAt string `json:"updated_at,omitempty"`
}

type User struct {
Username string `yaml:"username"`
Password string `yaml:"password"`
Salt string `yaml:"salt"`
Profile UserProfile `yaml:"profile"`
Address UserAddress `yaml:"address"`
}

type Claim struct {
Expand All @@ -83,7 +87,6 @@ type Client struct {
ClientSecret string `yaml:"clientSecret"`
Salt string `yaml:"salt"`
Oidc bool `yaml:"oidc"`
ClientType string `yaml:"type"`
AccessTTL int `yaml:"accessTTL"`
RefreshTTL int `yaml:"refreshTTL"`
IdTTL int `yaml:"idTTL"`
Expand Down Expand Up @@ -213,13 +216,18 @@ func (config *Config) Setup() error {
}

for clientIndex, client := range config.Clients {
if client.Id == "" || len(client.ClientSecret) != 128 {
invalidClient := fmt.Sprintf("Client configuration invalid. Client %d %v", clientIndex, client)
if client.Id == "" {
invalidClient := fmt.Sprintf("Client configuration invalid. Client %d is missing an client id, %v", clientIndex, client)
return errors.New(invalidClient)
}

if client.GetClientType() == oauth2.CtConfidential && len(client.ClientSecret) != 128 {
invalidClient := fmt.Sprintf("Client configuration invalid. Confidential client %d with id %s missing client secret, %v", clientIndex, client.Id, client)
return errors.New(invalidClient)
}

if len(client.Redirects) == 0 {
invalidClient := fmt.Sprintf("Client is missing redirects. Client %d %v", clientIndex, client)
invalidClient := fmt.Sprintf("Client is missing redirects. Client %d with id %s, %v", clientIndex, client.Id, client)
return errors.New(invalidClient)
}

Expand Down Expand Up @@ -321,3 +329,29 @@ func (client *Client) GetIssuer(requestData *internalHttp.RequestData) string {
func (client *Client) GetAudience() []string {
return GetOrDefaultStringSlice(client.Audience, []string{"all"})
}

func (client *Client) GetClientType() oauth2.ClientType {
if client.ClientSecret == "" {
return oauth2.CtPublic
} else {
return oauth2.CtConfidential
}
}

func (user *User) GetFormattedAddress() string {
userAddress := user.Profile.Address
var sb strings.Builder
if userAddress.Street != "" {
sb.WriteString(userAddress.Street)
sb.WriteString("\n")
}
if userAddress.PostalCode != "" {
sb.WriteString(userAddress.PostalCode)
sb.WriteString("\n")
}
if userAddress.City != "" {
sb.WriteString(userAddress.City)
sb.WriteString("\n")
}
return sb.String()
}
7 changes: 4 additions & 3 deletions internal/http/header.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package http

const (
Location string = "Location"
ContentType string = "Content-Type"
Authorization string = "Authorization"
Location string = "Location"
ContentType string = "Content-Type"
Authorization string = "Authorization"
AccessControlAllowOrigin string = "Access-Control-Allow-Origin"
)

const (
Expand Down
1 change: 1 addition & 0 deletions internal/http/header_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ func Test_HTTPHeaders(t *testing.T) {
var headerParameters = []headerParameter{
{Location, "Location"},
{ContentType, "Content-Type"},
{AccessControlAllowOrigin, "Access-Control-Allow-Origin"},
{Authorization, "Authorization"},
{AuthBasic, "Basic"},
{AuthBearer, "Bearer"},
Expand Down
1 change: 1 addition & 0 deletions internal/http/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ func SendJsonWithStatus(value any, w http.ResponseWriter, statusCode int) error
}

w.Header().Set(ContentType, ContentTypeJSON)
w.Header().Set(AccessControlAllowOrigin, "*")
w.WriteHeader(statusCode)
_, writeError := w.Write(bytes)
if writeError != nil {
Expand Down
1 change: 1 addition & 0 deletions internal/server/handler/oidc/userinfo.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ func (h *UserInfoHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
userInfoResponse = &user.Profile
userInfoResponse.Subject = user.Username
userInfoResponse.Name = userInfoResponse.GivenName + " " + userInfoResponse.FamilyName
userInfoResponse.Address.Formatted = user.GetFormattedAddress()
} else {
userInfoResponse = &config.UserProfile{}
}
Expand Down
11 changes: 8 additions & 3 deletions internal/server/handler/token/token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ func Test_Token(t *testing.T) {
Redirects: []string{"https://example.com/callback"},
RefreshTTL: 100,
},
{
Id: "bar",
Redirects: []string{"https://example.com/callback"},
RefreshTTL: 100,
},
},
Users: []config.User{
{
Expand Down Expand Up @@ -72,7 +77,7 @@ func Test_Token(t *testing.T) {
}

func testTokenMissingClientCredentials(t *testing.T, testConfig *config.Config, keyManager *manager.KeyManger) {
t.Run("Missing client credentials", func(t *testing.T) {
t.Run("Missing client credentials for confidential client", func(t *testing.T) {
requestValidator := validation.NewRequestValidator(testConfig)
sessionManager := manager.NewSessionManager(testConfig)
tokenManager := manager.NewTokenManager(testConfig, manager.NewDefaultKeyLoader(testConfig, keyManager))
Expand All @@ -83,8 +88,8 @@ func testTokenMissingClientCredentials(t *testing.T, testConfig *config.Config,

tokenHandler.ServeHTTP(rr, httptest.NewRequest(http.MethodPost, endpoint.Token, nil))

if rr.Code != http.StatusBadRequest {
t.Errorf("handler returned wrong status code: got %v want %v", rr.Code, http.StatusBadRequest)
if rr.Code != http.StatusUnauthorized {
t.Errorf("handler returned wrong status code: got %v want %v", rr.Code, http.StatusUnauthorized)
}
})
}
Expand Down
14 changes: 11 additions & 3 deletions internal/server/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,15 @@ func (validator *RequestValidator) ValidateClientCredentials(r *http.Request) (*
usingFallback := false
if !ok {
// Check usingFallback
log.Warn("Invalid or missing HTTP Basic authentication, using NOT RECOMMENDED usingFallback")
clientId = r.PostFormValue(oauth2.ParameterClientId)
clientSecret = r.PostFormValue(oauth2.ParameterClientSecret)
usingFallback = true
if clientId != "" {
log.Warn("Invalid or missing HTTP Basic authentication, using NOT RECOMMENDED POST from values")
usingFallback = true
}
}

if clientId == "" || clientSecret == "" {
if clientId == "" {
return nil, usingFallback, false
}

Expand All @@ -70,6 +72,12 @@ func (validator *RequestValidator) ValidateClientCredentials(r *http.Request) (*
return nil, usingFallback, false
}

if client.GetClientType() == oauth2.CtPublic && clientSecret == "" {
return client, usingFallback, true
} else if client.GetClientType() == oauth2.CtPublic && clientSecret != "" {
return nil, usingFallback, false
}

if !client.PasswordFallbackAllowed && usingFallback {
log.Warn("Client password usingFallback denied in configuration for client with id %v", client.Id)
return nil, usingFallback, false
Expand Down
20 changes: 16 additions & 4 deletions internal/server/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,21 @@ func Test_Validation(t *testing.T) {
valid bool
}

var passwordParameters = []passwordParameter{
var userPasswordParameters = []passwordParameter{
{name: "foo", password: "bar", valid: true},
{name: "foo", password: "xxx", valid: false},
{name: "bar", password: "xxx", valid: false},
{name: "", password: "", valid: false},
}

var clientPasswordParameters = []passwordParameter{
{name: "foo", password: "bar", valid: true},
{name: "foo", password: "xxx", valid: false},
{name: "bar", password: "xxx", valid: false},
{name: "moo", password: "", valid: true},
{name: "", password: "", valid: false},
}

var httpMethods = []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete}

testConfig := &config.Config{
Expand All @@ -37,6 +45,10 @@ func Test_Validation(t *testing.T) {
ClientSecret: "d82c4eb5261cb9c8aa9855edd67d1bd10482f41529858d925094d173fa662aa91ff39bc5b188615273484021dfb16fd8284cf684ccf0fc795be3aa2fc1e6c181",
Redirects: []string{"https://example.com/callback"},
},
{
Id: "moo",
Redirects: []string{"https://example.com/callback"},
},
},
Users: []config.User{
{
Expand All @@ -50,7 +62,7 @@ func Test_Validation(t *testing.T) {
t.Fatal(setupError)
}

for _, test := range passwordParameters {
for _, test := range userPasswordParameters {
testMessage := fmt.Sprintf("Valid user password %s %t", test.name, test.valid)
t.Run(testMessage, func(t *testing.T) {

Expand All @@ -64,7 +76,7 @@ func Test_Validation(t *testing.T) {
})
}

for _, test := range passwordParameters {
for _, test := range userPasswordParameters {
for _, httpMethod := range httpMethods {
testMessage := fmt.Sprintf("Valid user password from request %s %t %s", test.name, test.valid, httpMethod)
t.Run(testMessage, func(t *testing.T) {
Expand All @@ -89,7 +101,7 @@ func Test_Validation(t *testing.T) {
}
}

for _, test := range passwordParameters {
for _, test := range clientPasswordParameters {
for _, httpMethod := range httpMethods {
testMessage := fmt.Sprintf("Valid client credentials from request %s %t %s", test.name, test.valid, httpMethod)
t.Run(testMessage, func(t *testing.T) {
Expand Down

0 comments on commit be875ad

Please sign in to comment.