mirror of
https://github.com/vmware-tanzu/pinniped.git
synced 2026-09-26 18:04:24 +00:00
Add spec.secretName to OPC and handle case-insensitive hostnames
- When two different Issuers have the same host (i.e. they differ only by path) then they must have the same secretName. This is because it wouldn't make sense for there to be two different TLS certificates for one host. Find any that do not have the same secret name to put an error status on them and to avoid serving OIDC endpoints for them. The host comparison is case-insensitive. - Issuer hostnames should be treated as case-insensitive, because DNS hostnames are case-insensitive. So https://me.com and https://mE.cOm are duplicate issuers. However, paths are case-sensitive, so https://me.com/A and https://me.com/a are different issuers. Fixed this in the issuer validations and in the OIDC Manager's request router logic.
This commit is contained in:
Ryan Richard
@@ -6,6 +6,8 @@ package supervisorconfig
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"net/url"
|
||||
"strings"
|
||||
|
||||
"go.pinniped.dev/internal/multierror"
|
||||
|
||||
@@ -71,29 +73,76 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er
|
||||
return err
|
||||
}
|
||||
|
||||
// Make a map of issuer strings -> count of how many times we saw that issuer string.
|
||||
// This will help us complain when there are duplicate issuer strings.
|
||||
// Also make a helper function for forming keys into this map.
|
||||
issuerCounts := make(map[string]int)
|
||||
issuerURLToIssuerKey := func(issuerURL *url.URL) string {
|
||||
return fmt.Sprintf("%s://%s%s", issuerURL.Scheme, strings.ToLower(issuerURL.Host), issuerURL.Path)
|
||||
}
|
||||
|
||||
// Make a map of issuer addresses -> set of unique secret names. This will help us complain when
|
||||
// multiple OIDCProviderConfigs have the same issuer address (host) component but specify
|
||||
// different TLS serving Secrets. Doesn't make sense to have the one address use more than one
|
||||
// TLS cert. Also make a helper function for forming keys into this map.
|
||||
uniqueSecretNamesPerIssuerAddress := make(map[string]map[string]bool)
|
||||
issuerURLToHostKey := func(issuerURL *url.URL) string {
|
||||
return strings.ToLower(issuerURL.Host)
|
||||
}
|
||||
|
||||
for _, opc := range all {
|
||||
issuerCounts[opc.Spec.Issuer]++
|
||||
issuerURL, err := url.Parse(opc.Spec.Issuer)
|
||||
if err != nil {
|
||||
continue // Skip url parse errors because they will be validated again below.
|
||||
}
|
||||
|
||||
issuerCounts[issuerURLToIssuerKey(issuerURL)]++
|
||||
|
||||
setOfSecretNames := uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)]
|
||||
if setOfSecretNames == nil {
|
||||
setOfSecretNames = make(map[string]bool)
|
||||
uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)] = setOfSecretNames
|
||||
}
|
||||
setOfSecretNames[opc.Spec.SecretName] = true
|
||||
}
|
||||
|
||||
errs := multierror.New()
|
||||
|
||||
oidcProviders := make([]*provider.OIDCProvider, 0)
|
||||
for _, opc := range all {
|
||||
if issuerCount := issuerCounts[opc.Spec.Issuer]; issuerCount > 1 {
|
||||
issuerURL, urlParseErr := url.Parse(opc.Spec.Issuer)
|
||||
|
||||
// Skip url parse errors because they will be validated below.
|
||||
if urlParseErr == nil {
|
||||
if issuerCount := issuerCounts[issuerURLToIssuerKey(issuerURL)]; issuerCount > 1 {
|
||||
if err := c.updateStatus(
|
||||
ctx.Context,
|
||||
opc.Namespace,
|
||||
opc.Name,
|
||||
configv1alpha1.DuplicateOIDCProviderStatus,
|
||||
"Duplicate issuer: "+opc.Spec.Issuer,
|
||||
); err != nil {
|
||||
errs.Add(fmt.Errorf("could not update status: %w", err))
|
||||
}
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
// Skip url parse errors because they will be validated below.
|
||||
if urlParseErr == nil && len(uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)]) > 1 {
|
||||
if err := c.updateStatus(
|
||||
ctx.Context,
|
||||
opc.Namespace,
|
||||
opc.Name,
|
||||
configv1alpha1.DuplicateOIDCProviderStatus,
|
||||
"Duplicate issuer: "+opc.Spec.Issuer,
|
||||
configv1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus,
|
||||
"Issuers with the same address must use the same secretName: "+issuerURLToHostKey(issuerURL),
|
||||
); err != nil {
|
||||
errs.Add(fmt.Errorf("could not update status: %w", err))
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
oidcProvider, err := provider.NewOIDCProvider(opc.Spec.Issuer)
|
||||
oidcProvider, err := provider.NewOIDCProvider(opc.Spec.Issuer) // This validates the Issuer URL.
|
||||
if err != nil {
|
||||
if err := c.updateStatus(
|
||||
ctx.Context,
|
||||
|
||||
@@ -6,6 +6,7 @@ package supervisorconfig
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net/url"
|
||||
"reflect"
|
||||
"sync"
|
||||
"testing"
|
||||
@@ -667,22 +668,24 @@ func TestSync(t *testing.T) {
|
||||
)
|
||||
|
||||
it.Before(func() {
|
||||
// Hostnames are case-insensitive, so consider them to be duplicates if they only differ by case.
|
||||
// Paths are case-sensitive, so having a path that differs only by case makes a new issuer.
|
||||
oidcProviderConfigDuplicate1 = &v1alpha1.OIDCProviderConfig{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "duplicate1", Namespace: namespace},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com"},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://iSSueR-duPlicAte.cOm/a"},
|
||||
}
|
||||
r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigDuplicate1))
|
||||
r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigDuplicate1))
|
||||
oidcProviderConfigDuplicate2 = &v1alpha1.OIDCProviderConfig{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "duplicate2", Namespace: namespace},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com"},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com/a"},
|
||||
}
|
||||
r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigDuplicate2))
|
||||
r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigDuplicate2))
|
||||
|
||||
oidcProviderConfig = &v1alpha1.OIDCProviderConfig{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "not-duplicate", Namespace: namespace},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-not-duplicate.com"},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com/A"}, // different path
|
||||
}
|
||||
r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfig))
|
||||
r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfig))
|
||||
@@ -715,11 +718,11 @@ func TestSync(t *testing.T) {
|
||||
oidcProviderConfig.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
oidcProviderConfigDuplicate1.Status.Status = v1alpha1.DuplicateOIDCProviderStatus
|
||||
oidcProviderConfigDuplicate1.Status.Message = "Duplicate issuer: https://issuer-duplicate.com"
|
||||
oidcProviderConfigDuplicate1.Status.Message = "Duplicate issuer: https://iSSueR-duPlicAte.cOm/a"
|
||||
oidcProviderConfigDuplicate1.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
oidcProviderConfigDuplicate2.Status.Status = v1alpha1.DuplicateOIDCProviderStatus
|
||||
oidcProviderConfigDuplicate2.Status.Message = "Duplicate issuer: https://issuer-duplicate.com"
|
||||
oidcProviderConfigDuplicate2.Status.Message = "Duplicate issuer: https://issuer-duplicate.com/a"
|
||||
oidcProviderConfigDuplicate2.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
expectedActions := []coretesting.Action{
|
||||
@@ -770,10 +773,10 @@ func TestSync(t *testing.T) {
|
||||
|
||||
it("returns the get errors", func() {
|
||||
expectedError := here.Doc(`
|
||||
3 error(s):
|
||||
- could not update status: get failed: some get error
|
||||
- could not update status: get failed: some get error
|
||||
- could not update status: get failed: some get error`)
|
||||
3 error(s):
|
||||
- could not update status: get failed: some get error
|
||||
- could not update status: get failed: some get error
|
||||
- could not update status: get failed: some get error`)
|
||||
startInformersAndController()
|
||||
err := controllerlib.TestSync(t, subject, *syncContext)
|
||||
r.EqualError(err, expectedError)
|
||||
@@ -804,6 +807,196 @@ func TestSync(t *testing.T) {
|
||||
})
|
||||
})
|
||||
|
||||
when("there are OIDCProviderConfigs with the same issuer address using different secretNames", func() {
|
||||
var (
|
||||
oidcProviderConfigSameIssuerAddress1 *v1alpha1.OIDCProviderConfig
|
||||
oidcProviderConfigSameIssuerAddress2 *v1alpha1.OIDCProviderConfig
|
||||
oidcProviderConfigDifferentIssuerAddress *v1alpha1.OIDCProviderConfig
|
||||
oidcProviderConfigWithInvalidIssuerURL *v1alpha1.OIDCProviderConfig
|
||||
)
|
||||
|
||||
it.Before(func() {
|
||||
oidcProviderConfigSameIssuerAddress1 = &v1alpha1.OIDCProviderConfig{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "provider1", Namespace: namespace},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{
|
||||
Issuer: "https://iSSueR-duPlicAte-adDress.cOm/path1",
|
||||
SecretName: "secret1",
|
||||
},
|
||||
}
|
||||
r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigSameIssuerAddress1))
|
||||
r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigSameIssuerAddress1))
|
||||
oidcProviderConfigSameIssuerAddress2 = &v1alpha1.OIDCProviderConfig{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "provider2", Namespace: namespace},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{
|
||||
Issuer: "https://issuer-duplicate-address.com/path2",
|
||||
SecretName: "secret2",
|
||||
},
|
||||
}
|
||||
r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigSameIssuerAddress2))
|
||||
r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigSameIssuerAddress2))
|
||||
|
||||
oidcProviderConfigDifferentIssuerAddress = &v1alpha1.OIDCProviderConfig{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "differentIssuerAddressProvider", Namespace: namespace},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{
|
||||
Issuer: "https://issuer-not-duplicate.com",
|
||||
SecretName: "secret1",
|
||||
},
|
||||
}
|
||||
r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigDifferentIssuerAddress))
|
||||
r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigDifferentIssuerAddress))
|
||||
|
||||
// Also add one with a URL that cannot be parsed to make sure that the error handling
|
||||
// for the duplicate issuers and secret names are not confused by invalid URLs.
|
||||
invalidIssuerURL := ":/host//path"
|
||||
_, err := url.Parse(invalidIssuerURL) //nolint:staticcheck // Yes, this URL is intentionally invalid.
|
||||
r.Error(err)
|
||||
oidcProviderConfigWithInvalidIssuerURL = &v1alpha1.OIDCProviderConfig{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "invalidIssuerURLProvider", Namespace: namespace},
|
||||
Spec: v1alpha1.OIDCProviderConfigSpec{
|
||||
Issuer: invalidIssuerURL,
|
||||
SecretName: "secret1",
|
||||
},
|
||||
}
|
||||
r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigWithInvalidIssuerURL))
|
||||
r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigWithInvalidIssuerURL))
|
||||
})
|
||||
|
||||
it("calls the ProvidersSetter with the non-duplicate", func() {
|
||||
startInformersAndController()
|
||||
err := controllerlib.TestSync(t, subject, *syncContext)
|
||||
r.NoError(err)
|
||||
|
||||
nonDuplicateProvider, err := provider.NewOIDCProvider(oidcProviderConfigDifferentIssuerAddress.Spec.Issuer)
|
||||
r.NoError(err)
|
||||
|
||||
r.True(providersSetter.SetProvidersWasCalled)
|
||||
r.Equal(
|
||||
[]*provider.OIDCProvider{
|
||||
nonDuplicateProvider,
|
||||
},
|
||||
providersSetter.OIDCProvidersReceived,
|
||||
)
|
||||
})
|
||||
|
||||
it("updates the statuses", func() {
|
||||
startInformersAndController()
|
||||
err := controllerlib.TestSync(t, subject, *syncContext)
|
||||
r.NoError(err)
|
||||
|
||||
oidcProviderConfigDifferentIssuerAddress.Status.Status = v1alpha1.SuccessOIDCProviderStatus
|
||||
oidcProviderConfigDifferentIssuerAddress.Status.Message = "Provider successfully created"
|
||||
oidcProviderConfigDifferentIssuerAddress.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
oidcProviderConfigSameIssuerAddress1.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus
|
||||
oidcProviderConfigSameIssuerAddress1.Status.Message = "Issuers with the same address must use the same secretName: issuer-duplicate-address.com"
|
||||
oidcProviderConfigSameIssuerAddress1.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
oidcProviderConfigSameIssuerAddress2.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus
|
||||
oidcProviderConfigSameIssuerAddress2.Status.Message = "Issuers with the same address must use the same secretName: issuer-duplicate-address.com"
|
||||
oidcProviderConfigSameIssuerAddress2.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
oidcProviderConfigWithInvalidIssuerURL.Status.Status = v1alpha1.InvalidOIDCProviderStatus
|
||||
oidcProviderConfigWithInvalidIssuerURL.Status.Message = `Invalid: could not parse issuer as URL: parse ":/host//path": missing protocol scheme`
|
||||
oidcProviderConfigWithInvalidIssuerURL.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
expectedActions := []coretesting.Action{
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigSameIssuerAddress1.Namespace,
|
||||
oidcProviderConfigSameIssuerAddress1.Name,
|
||||
),
|
||||
coretesting.NewUpdateAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigSameIssuerAddress1.Namespace,
|
||||
oidcProviderConfigSameIssuerAddress1,
|
||||
),
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigSameIssuerAddress2.Namespace,
|
||||
oidcProviderConfigSameIssuerAddress2.Name,
|
||||
),
|
||||
coretesting.NewUpdateAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigSameIssuerAddress2.Namespace,
|
||||
oidcProviderConfigSameIssuerAddress2,
|
||||
),
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigDifferentIssuerAddress.Namespace,
|
||||
oidcProviderConfigDifferentIssuerAddress.Name,
|
||||
),
|
||||
coretesting.NewUpdateAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigDifferentIssuerAddress.Namespace,
|
||||
oidcProviderConfigDifferentIssuerAddress,
|
||||
),
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigWithInvalidIssuerURL.Namespace,
|
||||
oidcProviderConfigWithInvalidIssuerURL.Name,
|
||||
),
|
||||
coretesting.NewUpdateAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigWithInvalidIssuerURL.Namespace,
|
||||
oidcProviderConfigWithInvalidIssuerURL,
|
||||
),
|
||||
}
|
||||
r.ElementsMatch(expectedActions, pinnipedAPIClient.Actions())
|
||||
})
|
||||
|
||||
when("we cannot talk to the API", func() {
|
||||
it.Before(func() {
|
||||
pinnipedAPIClient.PrependReactor(
|
||||
"get",
|
||||
"oidcproviderconfigs",
|
||||
func(_ coretesting.Action) (bool, runtime.Object, error) {
|
||||
return true, nil, errors.New("some get error")
|
||||
},
|
||||
)
|
||||
})
|
||||
|
||||
it("returns the get errors", func() {
|
||||
expectedError := here.Doc(`
|
||||
4 error(s):
|
||||
- could not update status: get failed: some get error
|
||||
- could not update status: get failed: some get error
|
||||
- could not update status: get failed: some get error
|
||||
- could not update status: get failed: some get error`)
|
||||
startInformersAndController()
|
||||
err := controllerlib.TestSync(t, subject, *syncContext)
|
||||
r.EqualError(err, expectedError)
|
||||
|
||||
oidcProviderConfigDifferentIssuerAddress.Status.Status = v1alpha1.SuccessOIDCProviderStatus
|
||||
oidcProviderConfigDifferentIssuerAddress.Status.Message = "Provider successfully created"
|
||||
oidcProviderConfigDifferentIssuerAddress.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow))
|
||||
|
||||
expectedActions := []coretesting.Action{
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigSameIssuerAddress1.Namespace,
|
||||
oidcProviderConfigSameIssuerAddress1.Name,
|
||||
),
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigSameIssuerAddress2.Namespace,
|
||||
oidcProviderConfigSameIssuerAddress2.Name,
|
||||
),
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigDifferentIssuerAddress.Namespace,
|
||||
oidcProviderConfigDifferentIssuerAddress.Name,
|
||||
),
|
||||
coretesting.NewGetAction(
|
||||
oidcProviderConfigGVR,
|
||||
oidcProviderConfigWithInvalidIssuerURL.Namespace,
|
||||
oidcProviderConfigWithInvalidIssuerURL.Name,
|
||||
),
|
||||
}
|
||||
r.ElementsMatch(expectedActions, pinnipedAPIClient.Actions())
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
when("there are no OIDCProviderConfigs in the informer", func() {
|
||||
it("keeps waiting for one", func() {
|
||||
startInformersAndController()
|
||||
|
||||
Reference in New Issue
Block a user