-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add JWT-validation for idporten/maskinporten #589
base: main
Are you sure you want to change the base?
Changes from 28 commits
a479190
08aa9ee
deda127
c09baed
668c022
67e0a5a
b77e3ba
765266b
0d41d84
7df59e2
f100301
dede5e6
e1727c5
39d87e8
8b72d12
ce2b09c
67e4e56
8b68d05
bc81673
22bf348
758a1c8
41f0991
b38d667
c16c91c
28fc206
78e65c5
7cf2792
ebc585a
4eb2329
976b154
ec9e09d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package digdirator | ||
|
||
import ( | ||
"github.com/kartverket/skiperator/api/v1alpha1/istiotypes" | ||
v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
) | ||
|
||
type DigdiratorName string | ||
|
||
type DigdiratorURIs struct { | ||
Name DigdiratorName | ||
IssuerURI string | ||
JwksURI string | ||
ClientID string | ||
} | ||
|
||
type DigdiratorClient interface { | ||
GetOwnerReferences() []v1.OwnerReference | ||
GetSecretName() string | ||
} | ||
|
||
type DigdiratorProvider interface { | ||
IsEnabled() bool | ||
GetAuthSpec() istiotypes.Authentication | ||
GetDigdiratorName() DigdiratorName | ||
GetProvidedSecretName() *string | ||
GetPaths() []string | ||
GetIgnoredPaths() []string | ||
GetIssuerKey() string | ||
GetJwksKey() string | ||
GetClientIDKey() string | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,11 @@ | ||||||||||||||||||||||||||||||||||||||||||||
package digdirator | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
import nais_io_v1 "github.com/nais/liberator/pkg/apis/nais.io/v1" | ||||||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||||||
"github.com/kartverket/skiperator/api/v1alpha1/istiotypes" | ||||||||||||||||||||||||||||||||||||||||||||
"github.com/nais/digdirator/pkg/secrets" | ||||||||||||||||||||||||||||||||||||||||||||
nais_io_v1 "github.com/nais/liberator/pkg/apis/nais.io/v1" | ||||||||||||||||||||||||||||||||||||||||||||
v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// Based off NAIS' IDPorten specification as seen here: | ||||||||||||||||||||||||||||||||||||||||||||
// https://github.com/nais/liberator/blob/c9da4cf48a52c9594afc8a4325ff49bbd359d9d2/pkg/apis/nais.io/v1/naiserator_types.go#L93C10-L93C10 | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -75,4 +80,69 @@ type IDPorten struct { | |||||||||||||||||||||||||||||||||||||||||||
// +kubebuilder:validation:Minimum=3600 | ||||||||||||||||||||||||||||||||||||||||||||
// +kubebuilder:validation:Maximum=7200 | ||||||||||||||||||||||||||||||||||||||||||||
SessionLifetime *int `json:"sessionLifetime,omitempty"` | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
// Authentication specifies how incoming JWT's should be validated. | ||||||||||||||||||||||||||||||||||||||||||||
Authentication *istiotypes.Authentication `json:"authentication,omitempty"` | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
type IDPortenClient struct { | ||||||||||||||||||||||||||||||||||||||||||||
Client nais_io_v1.IDPortenClient | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPortenClient) GetSecretName() string { | ||||||||||||||||||||||||||||||||||||||||||||
return i.Client.Spec.SecretName | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPortenClient) GetOwnerReferences() []v1.OwnerReference { | ||||||||||||||||||||||||||||||||||||||||||||
return i.Client.GetOwnerReferences() | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
const IDPortenName DigdiratorName = "idporten" | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. variables on top of file |
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) IsEnabled() bool { | ||||||||||||||||||||||||||||||||||||||||||||
return i != nil && i.Enabled && i.Authentication != nil && i.Authentication.Enabled | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetAuthSpec() istiotypes.Authentication { | ||||||||||||||||||||||||||||||||||||||||||||
return *i.Authentication | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+106
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add nil check to prevent panic in GetAuthSpec. The function could panic if Apply this diff to add proper nil checks: func (i *IDPorten) GetAuthSpec() istiotypes.Authentication {
+ if i == nil || i.Authentication == nil {
+ return istiotypes.Authentication{}
+ }
return *i.Authentication
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetDigdiratorName() DigdiratorName { | ||||||||||||||||||||||||||||||||||||||||||||
return IDPortenName | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetProvidedSecretName() *string { | ||||||||||||||||||||||||||||||||||||||||||||
return i.Authentication.SecretName | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+114
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add nil check to prevent panic in GetProvidedSecretName. The function could panic if Apply this diff to add proper nil checks: func (i *IDPorten) GetProvidedSecretName() *string {
+ if i == nil || i.Authentication == nil {
+ return nil
+ }
return i.Authentication.SecretName
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetPaths() []string { | ||||||||||||||||||||||||||||||||||||||||||||
var paths []string | ||||||||||||||||||||||||||||||||||||||||||||
if i.IsEnabled() { | ||||||||||||||||||||||||||||||||||||||||||||
if i.Authentication.Paths != nil { | ||||||||||||||||||||||||||||||||||||||||||||
paths = append(paths, *i.Authentication.Paths...) | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
return paths | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetIgnoredPaths() []string { | ||||||||||||||||||||||||||||||||||||||||||||
var ignoredPaths []string | ||||||||||||||||||||||||||||||||||||||||||||
if i.IsEnabled() { | ||||||||||||||||||||||||||||||||||||||||||||
if i.Authentication.IgnorePaths != nil { | ||||||||||||||||||||||||||||||||||||||||||||
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
return ignoredPaths | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+128
to
+136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve error handling in GetIgnoredPaths. The function doesn't handle the case where Apply this diff to add proper nil checks: func (i *IDPorten) GetIgnoredPaths() []string {
var ignoredPaths []string
if i.IsEnabled() {
+ if i.Authentication == nil {
+ return ignoredPaths
+ }
if i.Authentication.IgnorePaths != nil {
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...)
}
}
return ignoredPaths
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetIssuerKey() string { | ||||||||||||||||||||||||||||||||||||||||||||
return secrets.IDPortenIssuerKey | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetJwksKey() string { | ||||||||||||||||||||||||||||||||||||||||||||
return secrets.IDPortenJwksUriKey | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
func (i *IDPorten) GetClientIDKey() string { | ||||||||||||||||||||||||||||||||||||||||||||
return secrets.IDPortenClientIDKey | ||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,11 @@ | ||||||||||||||||||||||||||||||||||||||
package digdirator | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import nais_io_v1 "github.com/nais/liberator/pkg/apis/nais.io/v1" | ||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||
"github.com/kartverket/skiperator/api/v1alpha1/istiotypes" | ||||||||||||||||||||||||||||||||||||||
"github.com/nais/digdirator/pkg/secrets" | ||||||||||||||||||||||||||||||||||||||
nais_io_v1 "github.com/nais/liberator/pkg/apis/nais.io/v1" | ||||||||||||||||||||||||||||||||||||||
v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// https://github.com/nais/liberator/blob/c9da4cf48a52c9594afc8a4325ff49bbd359d9d2/pkg/apis/nais.io/v1/naiserator_types.go#L376 | ||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||
|
@@ -15,4 +20,69 @@ type Maskinporten struct { | |||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// Schema to configure Maskinporten clients with consumed scopes and/or exposed scopes. | ||||||||||||||||||||||||||||||||||||||
Scopes *nais_io_v1.MaskinportenScope `json:"scopes,omitempty"` | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// Authentication specifies how incoming JWT's should be validated. | ||||||||||||||||||||||||||||||||||||||
Authentication *istiotypes.Authentication `json:"authentication,omitempty"` | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
type MaskinportenClient struct { | ||||||||||||||||||||||||||||||||||||||
Client nais_io_v1.MaskinportenClient | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (m *MaskinportenClient) GetOwnerReferences() []v1.OwnerReference { | ||||||||||||||||||||||||||||||||||||||
return m.Client.GetOwnerReferences() | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (m *MaskinportenClient) GetSecretName() string { | ||||||||||||||||||||||||||||||||||||||
return m.Client.Spec.SecretName | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
const MaskinPortenName = "maskinporten" | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. top |
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) IsEnabled() bool { | ||||||||||||||||||||||||||||||||||||||
return i != nil && i.Enabled && i.Authentication != nil && i.Authentication.Enabled | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication { | ||||||||||||||||||||||||||||||||||||||
return *i.Authentication | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix potential nil pointer dereference in GetAuthSpec. The method doesn't check if Apply this diff to fix the issue: func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication {
+ if i.Authentication == nil {
+ return istiotypes.Authentication{}
+ }
return *i.Authentication
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetDigdiratorName() DigdiratorName { | ||||||||||||||||||||||||||||||||||||||
return MaskinPortenName | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetProvidedSecretName() *string { | ||||||||||||||||||||||||||||||||||||||
return i.Authentication.SecretName | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+54
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix potential nil pointer dereference in GetProvidedSecretName. The method doesn't check if Apply this diff to fix the issue: func (i *Maskinporten) GetProvidedSecretName() *string {
+ if i.Authentication == nil {
+ return nil
+ }
return i.Authentication.SecretName
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetPaths() []string { | ||||||||||||||||||||||||||||||||||||||
var paths []string | ||||||||||||||||||||||||||||||||||||||
if i.IsEnabled() { | ||||||||||||||||||||||||||||||||||||||
if i.Authentication.Paths != nil { | ||||||||||||||||||||||||||||||||||||||
paths = append(paths, *i.Authentication.Paths...) | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
return paths | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetIgnoredPaths() []string { | ||||||||||||||||||||||||||||||||||||||
var ignoredPaths []string | ||||||||||||||||||||||||||||||||||||||
if i.IsEnabled() { | ||||||||||||||||||||||||||||||||||||||
if i.Authentication.IgnorePaths != nil { | ||||||||||||||||||||||||||||||||||||||
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...) | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
return ignoredPaths | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+68
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix potential nil pointer dereference in GetIgnoredPaths. The method accesses Apply this diff to fix the issue: func (i *Maskinporten) GetIgnoredPaths() []string {
var ignoredPaths []string
- if i.IsEnabled() {
+ if i.IsEnabled() && i.Authentication != nil {
if i.Authentication.IgnorePaths != nil {
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...)
}
}
return ignoredPaths
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetIssuerKey() string { | ||||||||||||||||||||||||||||||||||||||
return secrets.MaskinportenIssuerKey | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetJwksKey() string { | ||||||||||||||||||||||||||||||||||||||
return secrets.MaskinportenJwksUriKey | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
func (i *Maskinporten) GetClientIDKey() string { | ||||||||||||||||||||||||||||||||||||||
return secrets.MaskinportenClientIDKey | ||||||||||||||||||||||||||||||||||||||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
package istiotypes | ||
|
||
// Authentication specifies how incoming JWT's should be validated. | ||
// | ||
// +kubebuilder:object:generate=true | ||
type Authentication struct { | ||
// Whether to enable JWT validation. | ||
// If enabled, incoming JWT's will be validated against the issuer specified in the app registration and the generated audience. | ||
Enabled bool `json:"enabled"` | ||
|
||
// The name of the kubernetes Secret containing OAuth2-credentials. | ||
// | ||
// If omitted, the associated client registration in the application manifest is used for JWT validation. | ||
SecretName *string `json:"secretName,omitempty"` | ||
|
||
// If set to `true`, the original token will be kept for the upstream request. Defaults to `true`. | ||
// +kubebuilder:default=true | ||
ForwardOriginalToken bool `json:"forwardOriginalToken,omitempty"` | ||
|
||
// Where to find the JWT in the incoming request | ||
// | ||
// An enum value of `header` means that the JWT is present in the `Authorization`-header as a Bearer Token. | ||
// An enum value of `cookie` means that the JWT is present as a cookie called `BearerToken`. | ||
// | ||
// +kubebuilder:validation:Enum=header;cookie | ||
// +kubebuilder:default=header | ||
TokenLocation string `json:"tokenLocation,omitempty"` | ||
|
||
// This field specifies a list of operations to copy the claim to HTTP headers on a successfully verified token. | ||
// The header specified in each operation in the list must be unique. Nested claims of type string/int/bool is supported as well. | ||
// ``` | ||
// | ||
// outputClaimToHeaders: | ||
// - header: x-my-company-jwt-group | ||
// claim: my-group | ||
// - header: x-test-environment-flag | ||
// claim: test-flag | ||
// - header: x-jwt-claim-group | ||
// claim: nested.key.group | ||
// | ||
// ``` | ||
OutputClaimToHeaders *[]ClaimToHeader `json:"outputClaimToHeaders,omitempty"` | ||
larsore marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Paths specifies paths that require an authenticated JWT. | ||
// | ||
// The specified paths must start with '/'. | ||
// +listType=set | ||
// +kubebuilder:validation:Items.Pattern="^/" | ||
// +kubebuilder:validation:MaxItems=50 | ||
Paths *[]string `json:"paths,omitempty"` | ||
|
||
// IgnorePaths specifies paths that do not require an authenticated JWT. | ||
// | ||
// The specified paths must start with '/'. | ||
// +listType=set | ||
// +kubebuilder:validation:Items.Pattern="^/" | ||
// +kubebuilder:validation:MaxItems=50 | ||
IgnorePaths *[]string `json:"ignorePaths,omitempty"` | ||
} | ||
|
||
type ClaimToHeader struct { | ||
// The name of the HTTP-header for which the specified claim will be copied to. | ||
Header string `json:"header"` | ||
|
||
// The claim to be copied. | ||
Claim string `json:"claim"` | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add validation for URIs and ClientID.
The
DigdiratorURIs
struct contains critical security-related fields. Consider adding validation to ensure:IssuerURI
andJwksURI
are valid HTTPS URLsClientID
follows the expected format for the identity provider