diff --git a/.github/workflows/build_external_container_images.yaml b/.github/workflows/build_external_container_images.yaml index 7876dbfcb..7190c2f9d 100644 --- a/.github/workflows/build_external_container_images.yaml +++ b/.github/workflows/build_external_container_images.yaml @@ -3,14 +3,15 @@ name: Build Bitnami Container Images on: workflow_dispatch: -permissions: - contents: read - packages: write +permissions: read-all jobs: build_and_push_images: name: Build and Push ${{ matrix.image.name }} Image runs-on: ubuntu-latest + permissions: + contents: read + packages: write strategy: matrix: image: diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index b28cd78ff..2e20ae0e4 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -5,6 +5,8 @@ on: - cron: '30 1 * * *' workflow_dispatch: +permissions: {} + jobs: close-issues: runs-on: ubuntu-latest diff --git a/app/cli/main.go b/app/cli/main.go index 86da7300b..50bbcd0f7 100644 --- a/app/cli/main.go +++ b/app/cli/main.go @@ -16,6 +16,7 @@ package main import ( + "fmt" "os" "github.com/chainloop-dev/chainloop/app/cli/cmd" @@ -87,13 +88,22 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) { case v1.IsCasBackendErrorReasonInvalid(err): msg = "the CAS backend you provided is invalid. Refer to `chainloop cas-backend update` command or contact your administrator." case isWrappedErr(st, jwtMiddleware.ErrTokenExpired): - msg = "your authentication token has expired, please run chainloop auth login again" + msg = "your authentication token has expired, please run \"chainloop auth login\" again" + case isWrappedErr(st, jwtMiddleware.ErrTokenInvalid): + msg = "your authentication token is invalid, please run \"chainloop auth login\" again" + case isWrappedErr(st, jwtMiddleware.ErrTokenParseFail): + msg = "failed to parse authentication token, please run \"chainloop auth login\" again" case isWrappedErr(st, jwtMiddleware.ErrMissingJwtToken): msg = "authentication required, please run \"chainloop auth login\"" case v1.IsUserNotMemberOfOrgErrorNotInOrg(err): msg = "the organization you are trying to access does not exist or you are not part of it, please run \"chainloop auth login\"" case v1.IsUserWithNoMembershipErrorNotInOrg(err): msg = "you are not part of any organization, please run \"chainloop organization create --name ORG_NAME\" to create one" + case isUnmatchedAuthErr(st): + // Fallback for any other 401 errors not matched above. + // Must remain AFTER all specific Unauthenticated/org-membership cases + // to avoid shadowing them. + msg = fmt.Sprintf("authentication error: %s", st.Message()) case errors.As(err, &cmd.GracefulError{}): // Graceful recovery if the flag is set and the received error is marked as recoverable if cmd.GracefulExit { @@ -110,12 +120,21 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) { // target is the expected error // grpcStatus is the actual error that might be wrapped in both the status and the error +// +// NOTE: We intentionally do NOT use kratos errors.Is() here because it only +// compares Code and Reason. Since all JWT errors share the same Code (401) and +// Reason ("UNAUTHORIZED"), errors.Is() would match any 401 error against any +// JWT sentinel — causing e.g. "token invalid" to be reported as "token expired". +// Instead we compare Code and Message, which carry the distinguishing text. func isWrappedErr(grpcStatus *status.Status, target *errors.Error) bool { err := errors.FromError(grpcStatus.Err()) - // The error might be wrapped since the CLI sometimes returns a wrapped error - if errors.Is(err, target) { - return true - } - return target.Code == err.Code && err.Message == target.Message } + +// isUnmatchedAuthErr returns true when the gRPC status represents an +// Unauthenticated (401-equivalent) error that was not matched by any of the +// specific JWT sentinel checks above. This lets us surface the server's +// original message instead of silently dropping it. +func isUnmatchedAuthErr(grpcStatus *status.Status) bool { + return grpcStatus.Code() == codes.Unauthenticated +} diff --git a/app/cli/main_test.go b/app/cli/main_test.go new file mode 100644 index 000000000..93218245f --- /dev/null +++ b/app/cli/main_test.go @@ -0,0 +1,161 @@ +// +// Copyright 2023-2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "testing" + + kratosErrors "github.com/go-kratos/kratos/v2/errors" + jwtMiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt" + "github.com/stretchr/testify/assert" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// toGRPCStatus converts a Kratos *errors.Error into a *status.Status the same +// way it would travel over the wire: Kratos Error -> GRPCStatus() -> gRPC Status. +func toGRPCStatus(err *kratosErrors.Error) *status.Status { + return err.GRPCStatus() +} + +func TestIsWrappedErr(t *testing.T) { + tests := []struct { + name string + actual *kratosErrors.Error // the error that "came over the wire" + target *kratosErrors.Error // the sentinel we are checking against + want bool + }{ + { + name: "ErrTokenExpired matches itself", + actual: jwtMiddleware.ErrTokenExpired, + target: jwtMiddleware.ErrTokenExpired, + want: true, + }, + { + name: "ErrTokenInvalid matches itself", + actual: jwtMiddleware.ErrTokenInvalid, + target: jwtMiddleware.ErrTokenInvalid, + want: true, + }, + { + name: "ErrTokenParseFail matches itself", + actual: jwtMiddleware.ErrTokenParseFail, + target: jwtMiddleware.ErrTokenParseFail, + want: true, + }, + { + name: "ErrMissingJwtToken matches itself", + actual: jwtMiddleware.ErrMissingJwtToken, + target: jwtMiddleware.ErrMissingJwtToken, + want: true, + }, + { + name: "ErrTokenExpired does NOT match ErrTokenInvalid", + actual: jwtMiddleware.ErrTokenExpired, + target: jwtMiddleware.ErrTokenInvalid, + want: false, + }, + { + name: "ErrTokenInvalid does NOT match ErrTokenExpired", + actual: jwtMiddleware.ErrTokenInvalid, + target: jwtMiddleware.ErrTokenExpired, + want: false, + }, + { + name: "ErrTokenParseFail does NOT match ErrTokenExpired", + actual: jwtMiddleware.ErrTokenParseFail, + target: jwtMiddleware.ErrTokenExpired, + want: false, + }, + { + name: "ErrTokenExpired does NOT match ErrMissingJwtToken", + actual: jwtMiddleware.ErrTokenExpired, + target: jwtMiddleware.ErrMissingJwtToken, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + st := toGRPCStatus(tt.actual) + got := isWrappedErr(st, tt.target) + assert.Equal(t, tt.want, got, "isWrappedErr(%v, %v)", tt.actual.Message, tt.target.Message) + }) + } +} + +func TestIsUnmatchedAuthErr(t *testing.T) { + tests := []struct { + name string + st *status.Status + want bool + }{ + { + name: "generic 401/Unauthenticated error is caught", + st: status.New(codes.Unauthenticated, "some auth error"), + want: true, + }, + { + name: "JWT token expired (Unauthenticated) is caught", + st: toGRPCStatus(jwtMiddleware.ErrTokenExpired), + want: true, + }, + { + name: "PermissionDenied is NOT caught", + st: status.New(codes.PermissionDenied, "forbidden"), + want: false, + }, + { + name: "OK status is NOT caught", + st: status.New(codes.OK, ""), + want: false, + }, + { + name: "Internal error is NOT caught", + st: status.New(codes.Internal, "internal server error"), + want: false, + }, + { + name: "NotFound is NOT caught", + st: status.New(codes.NotFound, "not found"), + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isUnmatchedAuthErr(tt.st) + assert.Equal(t, tt.want, got, "isUnmatchedAuthErr()") + }) + } +} + +// TestKratosErrorsIsMasksJWTErrors demonstrates the bug that motivates our +// Message-based comparison: Kratos errors.Is() only compares Code and Reason. +// Since all JWT errors share Code=401 and Reason="UNAUTHORIZED", errors.Is() +// incorrectly matches ANY JWT error against ANY other JWT sentinel. +func TestKratosErrorsIsMasksJWTErrors(t *testing.T) { + // This test proves that the naive errors.Is approach is broken: + // ErrTokenExpired would incorrectly match ErrTokenInvalid via Kratos errors.Is. + if !kratosErrors.Is(jwtMiddleware.ErrTokenExpired, jwtMiddleware.ErrTokenInvalid) { + t.Skip("Kratos errors.Is behavior has changed; this test documents the original masking bug") + } + + // Now verify that our isWrappedErr correctly distinguishes them + st := toGRPCStatus(jwtMiddleware.ErrTokenExpired) + assert.False(t, isWrappedErr(st, jwtMiddleware.ErrTokenInvalid), "isWrappedErr should NOT match ErrTokenExpired against ErrTokenInvalid") + assert.True(t, isWrappedErr(st, jwtMiddleware.ErrTokenExpired), "isWrappedErr should match ErrTokenExpired against itself") +}