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

Add JWT-validation for idporten/maskinporten #589

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a479190
Extended IDPorten and Maskinporten definitions in skiperator spec to …
larsore Jan 8, 2025
08aa9ee
RequestAuthentication is now generated successfully, but missing the …
larsore Jan 9, 2025
deda127
Now generating RequestAuthentication which reads URI's from associate…
larsore Jan 16, 2025
c09baed
Merged defaultDeny AuthPolicy, AuthPolicy from AuthorizationSettings …
larsore Jan 17, 2025
668c022
Added tests
larsore Jan 21, 2025
67e0a5a
Made GetSecretData more general
larsore Jan 21, 2025
b77e3ba
Small changes to the tests
larsore Jan 21, 2025
765266b
Merge branch 'main' into add-jwt-authentication
larsore Jan 21, 2025
0d41d84
Switch from one auth policy to multiple
larsore Jan 22, 2025
7df59e2
Update AuthorizationPolicy name in subresource-status tests
larsore Jan 23, 2025
f100301
Update to pass unit tests
larsore Jan 24, 2025
dede5e6
Added error handling when no idporten/maskinportenclient is found
larsore Jan 29, 2025
e1727c5
Merge branch 'main' into add-jwt-authentication
larsore Jan 29, 2025
39d87e8
Update api/v1alpha1/istiotypes/jwt_authentication.go
larsore Jan 29, 2025
8b72d12
Refactor GetAuthConfigsForApplication
larsore Jan 30, 2025
ce2b09c
Refactored AuthConfig and split up authorization policy generator
larsore Feb 6, 2025
67e4e56
Fix type and add error check
larsore Feb 8, 2025
8b68d05
Refactor GetAuthConfigs WIP
larsore Feb 8, 2025
bc81673
Refactored the retrieval of auth configs to use interface for all pro…
larsore Feb 9, 2025
22bf348
Merge branch 'main' into add-jwt-authentication
larsore Feb 9, 2025
758a1c8
Renamed function and siplified if-statement
larsore Feb 9, 2025
41f0991
Refactored digdirator interface business logic inside application con…
larsore Feb 11, 2025
b38d667
Update on digdiratorProvider.IsEnabled implementations
larsore Feb 11, 2025
c16c91c
Merge branch 'main' into add-jwt-authentication
larsore Feb 11, 2025
28fc206
Pass authentication spec in auth config
larsore Feb 11, 2025
78e65c5
refactor (#601)
martinhny Feb 12, 2025
7cf2792
Include paths as input field to authentication
larsore Feb 13, 2025
ebc585a
Update auth policy rule generation
larsore Feb 13, 2025
4eb2329
Some refactoring and update on tests.
larsore Feb 17, 2025
976b154
Update test
larsore Feb 17, 2025
ec9e09d
Resolved comments and simplified auth config logic
larsore Feb 18, 2025
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
2 changes: 1 addition & 1 deletion api/v1alpha1/application_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ package v1alpha1
import (
"encoding/json"
"errors"
"github.com/kartverket/skiperator/api/v1alpha1/digdirator"
"time"

"github.com/kartverket/skiperator/api/v1alpha1/digdirator"
"github.com/kartverket/skiperator/api/v1alpha1/istiotypes"
"github.com/kartverket/skiperator/api/v1alpha1/podtypes"
corev1 "k8s.io/api/core/v1"
Expand Down
32 changes: 32 additions & 0 deletions api/v1alpha1/digdirator/digdirator.go
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
}
Comment on lines +10 to +15
Copy link
Contributor

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 and JwksURI are valid HTTPS URLs
  • ClientID follows the expected format for the identity provider


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
}
72 changes: 71 additions & 1 deletion api/v1alpha1/digdirator/idporten.go
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
Expand Down Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add nil check to prevent panic in GetAuthSpec.

The function could panic if Authentication is nil. Add a nil check before dereferencing.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (i *IDPorten) GetAuthSpec() istiotypes.Authentication {
return *i.Authentication
}
func (i *IDPorten) GetAuthSpec() istiotypes.Authentication {
if i == nil || i.Authentication == nil {
return istiotypes.Authentication{}
}
return *i.Authentication
}


func (i *IDPorten) GetDigdiratorName() DigdiratorName {
return IDPortenName
}

func (i *IDPorten) GetProvidedSecretName() *string {
return i.Authentication.SecretName
}
Comment on lines +114 to +116
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add nil check to prevent panic in GetProvidedSecretName.

The function could panic if Authentication is nil. Add a nil check before accessing the field.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (i *IDPorten) GetProvidedSecretName() *string {
return i.Authentication.SecretName
}
func (i *IDPorten) GetProvidedSecretName() *string {
if i == nil || i.Authentication == nil {
return nil
}
return i.Authentication.SecretName
}


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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling in GetIgnoredPaths.

The function doesn't handle the case where Authentication is nil but IsEnabled() returns true.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (i *IDPorten) GetIgnoredPaths() []string {
var ignoredPaths []string
if i.IsEnabled() {
if i.Authentication.IgnorePaths != nil {
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...)
}
}
return ignoredPaths
}
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
}


func (i *IDPorten) GetIssuerKey() string {
return secrets.IDPortenIssuerKey
}

func (i *IDPorten) GetJwksKey() string {
return secrets.IDPortenJwksUriKey
}

func (i *IDPorten) GetClientIDKey() string {
return secrets.IDPortenClientIDKey
}
72 changes: 71 additions & 1 deletion api/v1alpha1/digdirator/maskinporten.go
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
//
Expand All @@ -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"
Copy link
Contributor

@martinhny martinhny Feb 18, 2025

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential nil pointer dereference in GetAuthSpec.

The method doesn't check if Authentication is nil before dereferencing it.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication {
return *i.Authentication
}
func (i *Maskinporten) GetAuthSpec() istiotypes.Authentication {
if i.Authentication == nil {
return istiotypes.Authentication{}
}
return *i.Authentication
}


func (i *Maskinporten) GetDigdiratorName() DigdiratorName {
return MaskinPortenName
}

func (i *Maskinporten) GetProvidedSecretName() *string {
return i.Authentication.SecretName
}
Comment on lines +54 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential nil pointer dereference in GetProvidedSecretName.

The method doesn't check if Authentication is nil before accessing its fields.

Apply this diff to fix the issue:

 func (i *Maskinporten) GetProvidedSecretName() *string {
+    if i.Authentication == nil {
+        return nil
+    }
     return i.Authentication.SecretName
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (i *Maskinporten) GetProvidedSecretName() *string {
return i.Authentication.SecretName
}
func (i *Maskinporten) GetProvidedSecretName() *string {
if i.Authentication == nil {
return nil
}
return i.Authentication.SecretName
}


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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential nil pointer dereference in GetIgnoredPaths.

The method accesses i.Authentication.IgnorePaths without checking if i.Authentication is nil.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (i *Maskinporten) GetIgnoredPaths() []string {
var ignoredPaths []string
if i.IsEnabled() {
if i.Authentication.IgnorePaths != nil {
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...)
}
}
return ignoredPaths
}
func (i *Maskinporten) GetIgnoredPaths() []string {
var ignoredPaths []string
if i.IsEnabled() && i.Authentication != nil {
if i.Authentication.IgnorePaths != nil {
ignoredPaths = append(ignoredPaths, *i.Authentication.IgnorePaths...)
}
}
return ignoredPaths
}


func (i *Maskinporten) GetIssuerKey() string {
return secrets.MaskinportenIssuerKey
}

func (i *Maskinporten) GetJwksKey() string {
return secrets.MaskinportenJwksUriKey
}

func (i *Maskinporten) GetClientIDKey() string {
return secrets.MaskinportenClientIDKey
}
11 changes: 11 additions & 0 deletions api/v1alpha1/digdirator/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

67 changes: 67 additions & 0 deletions api/v1alpha1/istiotypes/jwt_authentication.go
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"`
}
47 changes: 47 additions & 0 deletions api/v1alpha1/istiotypes/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading