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

feat: support for Ory Network #133

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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 README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ To deploy the controller, edit the value of the `--hydra-url` argument in the
| ---------------------------- | -------- | ---------------------------------------------------------------------------------------------------------------- | ------------- | ---------------------------------------- |
| **hydra-url** | yes | ORY Hydra's service address | - | ` ory-hydra-admin.ory.svc.cluster.local` |
| **hydra-port** | no | ORY Hydra's service port | `4445` | `4445` |
| **endpoint** | no | ORY Hydra's client endpoint | `/clients` | `clients` |
| **tls-trust-store** | no | TLS cert path for hydra client | `""` | `/etc/ssl/certs/ca-certificates.crt` |
| **insecure-skip-verify** | no | Skip http client insecure verification | `false` | `true` or `false` |
| **namespace** | no | Namespace in which the controller should operate. Setting this will make the controller ignore other namespaces. | `""` | `"my-namespace"` |
Expand Down
14 changes: 14 additions & 0 deletions api/v1alpha1/oauth2client_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,20 @@ type HydraAdmin struct {
// value "off" will force this to be off even if
// `--forwarded-proto` is specified
ForwardedProto string `json:"forwardedProto,omitempty"`

// ApiKeySecretRef is an object to define the secret which contains
// Ory Network API Key
ApiKeySecretRef ApiKeySecretRef `json:"apiKeySecretRef,omitempty"`
}

// ApiKeySecretRef contains Secret details for the API Key
type ApiKeySecretRef struct {
// Name of the secret containing the API Key
Name string `json:"name,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Name string `json:"name,omitempty"`
Name string `json:"name"`

The whole object can be optional, but once given the name of the secret must be supplied.
Moreover, for the object to properly validated by k8s we need to add the annotations like here

	// +kubebuilder:validation:Type=object
	// +nullable
	// +optional
	//
	// Metadata is abritrary data
	Metadata apiextensionsv1.JSON `json:"metadata,omitempty"`

// Key of the secret for the API key
Key string `json:"key,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default value of the field can be defined in 2 ways:
1 - in the code using a const like we do for other variables
2 - define it in the CRD using // +kubebuilder:default=foo

// Namespace of the secret if different from hydra-maester controller
Namespace string `json:"namespace,omitempty"`
}

// OAuth2ClientSpec defines the desired state of OAuth2Client
Expand Down
16 changes: 16 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

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

17 changes: 17 additions & 0 deletions config/crd/bases/hydra.ory.sh_oauth2clients.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,23 @@ spec:
HydraAdmin is the optional configuration to use for managing
this client
properties:
apiKeySecretRef:
description:
ApiKeySecretRef is an object to define the secret which
contains Ory Network API Key
properties:
key:
description: Key of the secret for the API key
type: string
name:
description: Name of the secret containing the API Key
type: string
namespace:
description:
Namespace of the secret if different from
hydra-maester controller
type: string
type: object
endpoint:
description:
Endpoint is the endpoint for the hydra instance on which
Expand Down
39 changes: 39 additions & 0 deletions config/samples/hydra_v1alpha1_oauth2client_ory_network.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
apiVersion: hydra.ory.sh/v1alpha1
kind: OAuth2Client
metadata:
name: my-oauth2-client
namespace: default
spec:
grantTypes:
- client_credentials
- implicit
- authorization_code
- refresh_token
responseTypes:
- id_token
- code
- token
- code token
- code id_token
- id_token token
- code id_token token
scope: "read write"
secretName: my-secret-123
# these are optional
redirectUris:
- https://client/account
- http://localhost:8080
postLogoutRedirectUris:
- https://client/logout
audience:
- audience-a
- audience-b
hydraAdmin:
# if hydraAdmin is specified, all of these fields are requried,
# but they can be empty/0
url: https://foobar.projects.oryapis.com
endpoint: /admin/clients
port: 443
apiKeySecretRef:
name: ory_network_key
tokenEndpointAuthMethod: client_secret_basic
44 changes: 40 additions & 4 deletions controllers/oauth2client_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,16 @@ const (
)

type clientKey struct {
url string
port int
endpoint string
forwardedProto string
url string
port int
endpoint string
forwardedProto string
apiKeySecretRef ApiKeySecretRef
}

type ApiKeySecretRef struct {
Name string
Namespace string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you forget about the Key here ;d?

}

// OAuth2ClientFactory is a function that creates oauth2 client.
Expand Down Expand Up @@ -413,6 +419,22 @@ func (r *OAuth2ClientReconciler) getHydraClientForClient(
endpoint: spec.HydraAdmin.Endpoint,
forwardedProto: spec.HydraAdmin.ForwardedProto,
}

secretName := determineApiSecretName(&spec.HydraAdmin)
secretNamespace := determineApiSecretNamespace(&oauth2client)

if secretName != "" && secretNamespace != "" {
key.apiKeySecretRef = ApiKeySecretRef{
Name: secretName,
Namespace: secretNamespace,
}
spec.HydraAdmin.ApiKeySecretRef.Name = secretName
spec.HydraAdmin.ApiKeySecretRef.Namespace = secretNamespace
if spec.HydraAdmin.ApiKeySecretRef.Key == "" {
spec.HydraAdmin.ApiKeySecretRef.Key = "hydra_api_key"
Copy link
Collaborator

Choose a reason for hiding this comment

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

for consistency please move the default key value to consts and refer to it here :)

}
}

r.mu.Lock()
defer r.mu.Unlock()
if c, ok := r.oauth2Clients[key]; ok {
Expand Down Expand Up @@ -457,3 +479,17 @@ func removeString(slice []string, s string) (result []string) {
}
return
}

func determineApiSecretName(spec *hydrav1alpha1.HydraAdmin) string {
if spec.ApiKeySecretRef.Name != "" {
return spec.ApiKeySecretRef.Name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the name should be always given, this shouldn't be required, or throw an error if the name is empty

}
return ""
}

func determineApiSecretNamespace(spec *hydrav1alpha1.OAuth2Client) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would make more sense as a method on the OAuth2Client type

if spec.Spec.HydraAdmin.ApiKeySecretRef.Namespace != "" {
return spec.Spec.HydraAdmin.ApiKeySecretRef.Namespace
}
return spec.ObjectMeta.Namespace
}
60 changes: 60 additions & 0 deletions hydra/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,22 @@ package hydra

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"os"
"path"

hydrav1alpha1 "github.com/ory/hydra-maester/api/v1alpha1"
"github.com/ory/hydra-maester/helpers"
apiv1 "k8s.io/api/core/v1"
apierrs "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
)

type Client interface {
Expand All @@ -28,6 +35,7 @@ type InternalClient struct {
HydraURL url.URL
HTTPClient *http.Client
ForwardedProto string
ApiKey string
}

// New returns a new hydra InternalClient instance.
Expand All @@ -52,6 +60,18 @@ func New(spec hydrav1alpha1.OAuth2ClientSpec, tlsTrustStore string, insecureSkip
client.ForwardedProto = spec.HydraAdmin.ForwardedProto
}

apiKey, err := fetchApiKey(spec.HydraAdmin.ApiKeySecretRef)
if err != nil {
return nil, err
}

getEnv := os.Getenv("HYDRA_API_KEY")
if getEnv != "" {
client.ApiKey = getEnv
} else if apiKey != "" {
client.ApiKey = apiKey
}

return client, nil
}

Expand Down Expand Up @@ -186,6 +206,10 @@ func (c *InternalClient) newRequest(method, relativePath string, body interface{
req.Header.Add("X-Forwarded-Proto", c.ForwardedProto)
}

if c.ApiKey != "" {
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", c.ApiKey))
}

if body != nil {
req.Header.Set("Content-Type", "application/json")
}
Expand All @@ -207,3 +231,39 @@ func (c *InternalClient) do(req *http.Request, v interface{}) (*http.Response, e
}
return resp, err
}

func fetchApiKey(spec hydrav1alpha1.ApiKeySecretRef) (string, error) {

secretName := spec.Name
secretNamespace := spec.Namespace

if secretName == "" || secretNamespace == "" {
return "", nil
}

cfg := ctrl.GetConfigOrDie()
kubeClient, err := client.New(cfg, client.Options{})
Comment on lines +244 to +245
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a fan of this pattern. I think we should define the clients either in the New function, or pass them to the function to avoid the possibility of creating multiple instances. Then we can refactor this function into a method and create some unit tests :)

if err != nil {
ctrl.Log.Error(err, "unable to create client to fetch secret")
return "", err
}

var secret apiv1.Secret
err = kubeClient.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: secretNamespace}, &secret)
if err != nil {
if apierrs.IsNotFound(err) {
ctrl.Log.Error(err, fmt.Sprintf("secret %s/%s does not exist", secretName, secretNamespace))
return "", fmt.Errorf("secret %s/%s does not exist", secretNamespace, secretName)
}
ctrl.Log.Error(err, fmt.Sprintf("error fetching secret %s/%s", secretName, secretNamespace))
return "", fmt.Errorf("error fetching secret %s/%s: %s", secretNamespace, secretName, err)
}

secretValue, ok := secret.Data[spec.Key]
if !ok {
ctrl.Log.Error(err, fmt.Sprintf("key %s doesn't exist within secret %s/%s", secretName, secretNamespace, spec.Key))
return "", fmt.Errorf("key %s doesn't exist within secret %s/%s", secretName, secretNamespace, spec.Key)
}

return string(secretValue), nil
}