Merge branch 'branch/v15' into gabrielcorado/v15/fix-skip-idle-events…

…-recording

Makefile

@@ -1419,7 +1419,7 @@ derive:
 .PHONY: derive-up-to-date
 derive-up-to-date: must-start-clean/host derive
 	@if ! $(GIT) diff --quiet; then \
-		echo 'Please run make derive.'; \
+		./build.assets/please-run.sh "derived functions" "make derive"; \
 		exit 1; \
 	fi
 
@@ -1454,14 +1454,15 @@ endif
 .PHONY: protos-up-to-date/host
 protos-up-to-date/host: must-start-clean/host grpc/host
 	@if ! $(GIT) diff --quiet; then \
-		echo 'Please run make grpc.'; \
+		./build.assets/please-run.sh "protos gRPC" "make grpc"; \
 		exit 1; \
 	fi
 
 .PHONY: must-start-clean/host
 must-start-clean/host:
 	@if ! $(GIT) diff --quiet; then \
-		echo 'This must be run from a repo with no unstaged commits.'; \
+		@echo 'This must be run from a repo with no unstaged commits.'; \
+		git diff; \
 		exit 1; \
 	fi
 
@@ -1470,7 +1471,7 @@ must-start-clean/host:
 crds-up-to-date: must-start-clean/host
 	$(MAKE) -C integrations/operator manifests
 	@if ! $(GIT) diff --quiet; then \
-		echo 'Please run make -C integrations/operator manifests.'; \
+		./build.assets/please-run.sh "operator CRD manifests" "make -C integrations/operator crd"; \
 		exit 1; \
 	fi
 

build.assets/please-run.sh

@@ -0,0 +1,40 @@
+#!/bin/sh
+
+# This script is a helper that tells developers what generated content is out of date
+# and which command to run.
+# When running on GitHub actions, the script will also create an error in the PR and
+# collapse the diff to improve readability.
+
+set -eu
+
+# only echoes the string if we are in GitHub Actions
+echo_gha() {
+  [ -n "${GITHUB_ACTIONS+x}" ] && echo "$@"
+}
+
+main() {
+  if [ $# -ne 2 ]; then
+    echo "Usage: $0 <kind> <generate command>" >&2
+    exit 1
+  fi
+
+  KIND="$1"
+  GENERATE_COMMAND="$2"
+
+  TITLE="$KIND are out-of-date"
+  MESSAGE="Please run the command \`$GENERATE_COMMAND\`"
+
+  # Create a GitHub error
+  echo_gha "::error file=Makefile,title=$TITLE::$MESSAGE"
+
+  echo "============="
+  echo "$TITLE"
+  echo "$MESSAGE"
+  echo "============="
+
+  echo_gha "::group::Diff output"
+  git diff || true
+  echo_gha "::endgroup::"
+}
+
+main "$@"

integrations/operator/Makefile

@@ -72,6 +72,9 @@ help: ## Display this help.
 
 ##@ Development
 
+.PHONY: crd ## Single command to generate anything CRD-related (only manifests for this branch)
+crd: crdgen
+
 .PHONY: crdgen
 crdgen: ## Generate CRDs
 	make -C crdgen
@@ -119,6 +122,10 @@ test: export KUBEBUILDER_ASSETS=$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p
 test:
 	go test ./... -coverprofile cover.out
 
+.PHONY: echo-kubebuilder-assets
+echo-kubebuilder-assets:
+	@echo KUBEBUILDER_ASSETS=$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)
+
 .PHONY: crdgen-test
 crdgen-test: ## Run crdgen tests.
 	make -C crdgen test

integrations/operator/apis/resources/v3/oidcconnector_types.go

@@ -21,6 +21,7 @@ package v3
 import (
 	"encoding/json"
 
+	"github.com/gravitational/trace"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/gravitational/teleport/api/types"
@@ -97,14 +98,49 @@ func (spec *TeleportOIDCConnectorSpec) DeepCopyInto(out *TeleportOIDCConnectorSp
 	}
 }
 
+// Custom json.Marshaller and json.Unmarshaler are here to cope with inconsistencies between our CRD and go types.
+// They are invoked when the kubernetes client converts the unstructured object into a typed resource.
+// We have two inconsistencies:
+// - the utils.Strings typr that marshals inconsistently: single elements are strings, multiple elements are lists
+// - the max_age setting which is an embedded pointer to another single-value message, which breaks JSON parsing
+
 // MarshalJSON serializes a spec into a JSON string
 func (spec TeleportOIDCConnectorSpec) MarshalJSON() ([]byte, error) {
 	type Alias TeleportOIDCConnectorSpec
+
+	var maxAge types.Duration
+	if spec.MaxAge != nil {
+		maxAge = spec.MaxAge.Value
+	}
+
 	return json.Marshal(&struct {
-		RedirectURLs []string `json:"redirect_url"`
+		RedirectURLs []string       `json:"redirect_url,omitempty"`
+		MaxAge       types.Duration `json:"max_age,omitempty"`
 		Alias
 	}{
 		RedirectURLs: spec.RedirectURLs,
+		MaxAge:       maxAge,
 		Alias:        (Alias)(spec),
 	})
 }
+
+// UnmarshalJSON serializes a JSON string into a spec. This override is required to deal with the
+// MaxAge field which is special case because it' an object embedded into the spec.
+func (spec *TeleportOIDCConnectorSpec) UnmarshalJSON(data []byte) error {
+	*spec = *new(TeleportOIDCConnectorSpec)
+	type Alias TeleportOIDCConnectorSpec
+
+	temp := &struct {
+		MaxAge types.Duration `json:"max_age"`
+		*Alias
+	}{
+		Alias: (*Alias)(spec),
+	}
+	if err := json.Unmarshal(data, &temp); err != nil {
+		return trace.Wrap(err, "unmarshalling custom teleport oidc connector spec")
+	}
+	if temp.MaxAge != 0 {
+		spec.MaxAge = &types.MaxAge{Value: temp.MaxAge}
+	}
+	return nil
+}

integrations/operator/apis/resources/v3/oidcconnector_types_test.go

@@ -21,9 +21,11 @@ package v3
 import (
 	"encoding/json"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/api/types/wrappers"
 )
 
@@ -50,6 +52,11 @@ func TestTeleportOIDCConnectorSpec_MarshalJSON(t *testing.T) {
 			TeleportOIDCConnectorSpec{RedirectURLs: wrappers.Strings{"foo", "bar"}},
 			`{"redirect_url":["foo","bar"],"issuer_url":"","client_id":"","client_secret":""}`,
 		},
+		{
+			"MaxAge",
+			TeleportOIDCConnectorSpec{MaxAge: &types.MaxAge{Value: types.Duration(time.Hour)}},
+			`{"max_age":"1h0m0s","issuer_url":"","client_id":"","client_secret":""}`,
+		},
 	}
 	for _, tc := range tests {
 		tc := tc
@@ -60,3 +67,39 @@ func TestTeleportOIDCConnectorSpec_MarshalJSON(t *testing.T) {
 		})
 	}
 }
+func TestTeleportOIDCConnectorSpec_UnmarshalJSON(t *testing.T) {
+	tests := []struct {
+		name         string
+		expectedSpec TeleportOIDCConnectorSpec
+		inputJSON    string
+	}{
+		{
+			"Empty string",
+			TeleportOIDCConnectorSpec{RedirectURLs: wrappers.Strings{""}},
+			`{"redirect_url":[""],"issuer_url":"","client_id":"","client_secret":""}`,
+		},
+		{
+			"Single string",
+			TeleportOIDCConnectorSpec{RedirectURLs: wrappers.Strings{"foo"}},
+			`{"redirect_url":["foo"],"issuer_url":"","client_id":"","client_secret":""}`,
+		},
+		{
+			"Multiple strings",
+			TeleportOIDCConnectorSpec{RedirectURLs: wrappers.Strings{"foo", "bar"}},
+			`{"redirect_url":["foo","bar"],"issuer_url":"","client_id":"","client_secret":""}`,
+		},
+		{
+			"MaxAge",
+			TeleportOIDCConnectorSpec{MaxAge: &types.MaxAge{Value: types.Duration(time.Hour)}},
+			`{"max_age":"1h0m0s","issuer_url":"","client_id":"","client_secret":""}`,
+		},
+	}
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			var spec TeleportOIDCConnectorSpec
+			require.NoError(t, json.Unmarshal([]byte(tc.inputJSON), &spec))
+			require.Equal(t, tc.expectedSpec, spec)
+		})
+	}
+}

integrations/operator/controllers/resources/oidc_connector_controller_test.go

@@ -21,6 +21,7 @@ package resources_test
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/gravitational/trace"
@@ -43,6 +44,7 @@ var oidcSpec = types.OIDCConnectorSpecV3{
 		Roles: []string{"roleA"},
 	}},
 	RedirectURLs: []string{"https://redirect"},
+	MaxAge:       &types.MaxAge{Value: types.Duration(time.Hour)},
 }
 
 type oidcTestingPrimitives struct {

lib/auth/webauthncli/fido2.go

@@ -25,6 +25,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/base64"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -220,15 +221,15 @@ func fido2Login(
 		if uv {
 			opts.UV = libfido2.True
 		}
-		assertions, err := dev.Assertion(actualRPID, ccdHash[:], allowedCreds, pin, opts)
+		assertions, err := devAssertion(dev, info, actualRPID, ccdHash[:], allowedCreds, pin, opts)
 		if errors.Is(err, libfido2.ErrUnsupportedOption) && uv && pin != "" {
 			// Try again if we are getting "unsupported option" and the PIN is set.
 			// Happens inconsistently in some authenticator series (YubiKey 5).
 			// We are relying on the fact that, because the PIN is set, the
 			// authenticator will set the UV bit regardless of it being requested.
 			log.Debugf("FIDO2: Device %v: retrying assertion without UV", info.path)
 			opts.UV = libfido2.Default
-			assertions, err = dev.Assertion(actualRPID, ccdHash[:], allowedCreds, pin, opts)
+			assertions, err = devAssertion(dev, info, actualRPID, ccdHash[:], allowedCreds, pin, opts)
 		}
 		if errors.Is(err, libfido2.ErrNoCredentials) {
 			// U2F devices error instantly with ErrNoCredentials.
@@ -312,13 +313,75 @@ func usesAppID(dev FIDODevice, info *deviceInfo, ccdHash []byte, allowedCreds []
 
 	isRegistered := func(id string) bool {
 		const pin = "" // Not necessary here.
-		_, err := dev.Assertion(id, ccdHash, allowedCreds, pin, opts)
+		_, err := devAssertion(dev, info, id, ccdHash, allowedCreds, pin, opts)
 		return err == nil || (!info.fido2 && errors.Is(err, libfido2.ErrUserPresenceRequired))
 	}
 
 	return isRegistered(appID) && !isRegistered(rpID)
 }
 
+func devAssertion(
+	dev FIDODevice,
+	info *deviceInfo,
+	rpID string,
+	ccdHash []byte,
+	allowedCreds [][]byte,
+	pin string,
+	opts *libfido2.AssertionOpts,
+) ([]*libfido2.Assertion, error) {
+	// Handle U2F devices separately when there is more than one allowed
+	// credential.
+	// This avoids "internal errors" on older Yubikey models (eg, FIDO U2F
+	// Security Key firmware 4.1.8).
+	if !info.fido2 && len(allowedCreds) > 1 {
+		cred, ok := findFirstKnownCredential(dev, info, rpID, ccdHash, allowedCreds)
+		if ok {
+			isCredentialCheck := pin == "" && opts != nil && opts.UP == libfido2.False
+			if isCredentialCheck {
+				// No need to assert again, reply as the U2F authenticator would.
+				return nil, trace.Wrap(libfido2.ErrUserPresenceRequired)
+			}
+
+			if log.IsLevelEnabled(log.DebugLevel) {
+				credPrefix := hex.EncodeToString(cred)
+				const prefixLen = 10
+				if len(credPrefix) > prefixLen {
+					credPrefix = credPrefix[:prefixLen]
+				}
+				log.Debugf("FIDO2: Device %v: Using credential %v...", info.path, credPrefix)
+			}
+
+			allowedCreds = [][]byte{cred}
+		}
+	}
+
+	assertion, err := dev.Assertion(rpID, ccdHash, allowedCreds, pin, opts)
+	return assertion, trace.Wrap(err)
+}
+
+func findFirstKnownCredential(
+	dev FIDODevice,
+	info *deviceInfo,
+	rpID string,
+	ccdHash []byte,
+	allowedCreds [][]byte,
+) ([]byte, bool) {
+	const pin = ""
+	opts := &libfido2.AssertionOpts{
+		UP: libfido2.False,
+	}
+	for _, cred := range allowedCreds {
+		_, err := dev.Assertion(rpID, ccdHash, [][]byte{cred}, pin, opts)
+		// FIDO2 devices return err=nil on up=false queries; U2F devices return
+		// libfido2.ErrUserPresenceRequired.
+		// https://github.com/Yubico/libfido2/blob/03c18d396eb209a42bbf62f5f4415203cba2fc50/src/u2f.c#L787-L791.
+		if err == nil || (!info.fido2 && errors.Is(err, libfido2.ErrUserPresenceRequired)) {
+			return cred, true
+		}
+	}
+	return nil, false
+}
+
 func pickAssertion(
 	assertions []*libfido2.Assertion, prompt LoginPrompt, user string, passwordless bool,
 ) (*libfido2.Assertion, error) {
@@ -452,7 +515,7 @@ func fido2Register(
 
 		// Does the device hold an excluded credential?
 		const pin = "" // not required to filter
-		switch _, err := dev.Assertion(rp.ID, ccdHash[:], excludeList, pin, &libfido2.AssertionOpts{
+		switch _, err := devAssertion(dev, info, rp.ID, ccdHash[:], excludeList, pin, &libfido2.AssertionOpts{
 			UP: libfido2.False,
 		}); {
 		case errors.Is(err, libfido2.ErrNoCredentials):