-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
v3.5.5 unknown name groups
during SSO
#13061
Comments
Thanks for the detailed report!
Specifically, the only SSO change in v3.5.5 was #12318, which is unrelated in this case (different provider, different claim). So my suspicion would then be that a deps change somehow impacted this, and #12573 upgraded
@Freddybob4244 could you provide a few more details that I had asked for in my last message:
Also can you provide the admin SA with its
From the same message, could you try some of these options and check what the logs say:
I'm not sure if we have a robust test environment for SSO issues; we do have an optional Dex but I'm not sure if it's configured and used for SSO tests (I don't think it is IIRC). That would be great to have for reproducibility and to add test cases for the pieces of SSO that are not provider specific. |
unknown name groups
during SSO in v3.5.5
From DMs:
That seems to confirm that #12573 is the source of the issue here, now we need to figure out why it's causing this exactly and how to fix that (and if it requires another upstream fix in Also that |
Confirmed with a different user in another Slack thread that this is a regression in 3.5.5 and that reverting to 3.5.4 fixed it |
@isubasinghe any chance you could take a look at this? Related to the |
unknown name groups
during SSO in v3.5.5unknown name groups
during SSO
Let's fix the error.
Which seems legit. |
Can you show an example expression with an error? |
@antonmedv https://github.com/argoproj/argo-workflows/pull/12573/files#diff-ace0e536d9a8878e3c778026bd37f97d15bd3d47f889ad972704a2a2ba11878a was changed at same time as the version upgrade. it is called from argo-workflows/server/auth/gatekeeper.go Line 264 in 06da23e
|
The error is
Which means there is no |
i tried below in go playground (https://go.dev/play/) but could not reproduce the error: package main
import (
"encoding/json"
"fmt"
"github.com/antonmedv/expr"
"gopkg.in/square/go-jose.v2/jwt"
)
type CustomClaims struct {
Issuer string `json:"iss,omitempty"`
Subject string `json:"sub,omitempty"`
Audience jwt.Audience `json:"aud,omitempty"`
Expiry *jwt.NumericDate `json:"exp,omitempty"`
NotBefore *jwt.NumericDate `json:"nbf,omitempty"`
IssuedAt *jwt.NumericDate `json:"iat,omitempty"`
ID string `json:"jti,omitempty"`
Groups []string `json:"groups,omitempty"`
}
func Jsonify(v interface{}) (map[string]interface{}, error) {
data, err := json.Marshal(v)
if err != nil {
return nil, err
}
x := make(map[string]interface{})
return x, json.Unmarshal(data, &x)
}
func main() {
//claims
//data, err := json.Marshal(claims)
// if err != nil {
// panic(err)
// }
// v := make(map[string]interface{}) //env
// err = json.Unmarshal(data, &v)
// if err != nil {
// panic(err)
// }
//env := map[string]interface{}{
// "groups": []string{"abc", "ac1ef805-ac6a-4ce9-854a-1cb406aa7121"},
//}
//type Claims struct {
// jwt.Claims
// Groups []string `json:"groups,omitempty"`
// Email string `json:"email,omitempty"`
// EmailVerified bool `json:"-"`
// Name string `json:"name,omitempty"`
/// ServiceAccountName string `json:"service_account_name,omitempty"`
// ServiceAccountNamespace string `json:"service_account_namespace,omitempty"`
// PreferredUsername string `json:"preferred_username,omitempty"`
// RawClaim map[string]interface{} `json:"-"`
// }
//claims := Claims{
// Claims: jwt.Claims{
// // Initialize fields of jwt.Claims as needed
// },
// Groups: []string{"abc", "ac1ef805-ac6a-4ce9-854a-1cb406aa7121"},
//}
type Claims struct {
CustomClaims
Email string `json:"email,omitempty"`
EmailVerified bool `json:"-"`
Name string `json:"name,omitempty"`
ServiceAccountName string `json:"service_account_name,omitempty"`
ServiceAccountNamespace string `json:"service_account_namespace,omitempty"`
PreferredUsername string `json:"preferred_username,omitempty"`
RawClaim map[string]interface{} `json:"-"`
}
claims := Claims{
CustomClaims: CustomClaims{
Groups: []string{"abc", "ac1ef805-ac6a-4ce9-854a-1cb406aa7121"},
},
}
//claims := Claims{Claims: jwt.Claims{"Groups": []string{"abc", "ac1ef805-ac6a-4ce9-854a-1cb406aa7121"}}}
v, err := Jsonify(claims)
if err != nil {
panic(err)
}
fmt.Println(v)
input := `'ac1ef805-ac6a-4ce9-854a-1cb406aa7121' in groups
` //rule
result, err := expr.Eval(input, v)
if err != nil {
panic(err)
}
fmt.Println(result)
}
-- go.mod --
module play.ground
require github.com/antonmedv/expr v1.15.5 package main
import (
"encoding/json"
"fmt"
"github.com/expr-lang/expr"
"gopkg.in/square/go-jose.v2/jwt"
)
type CustomClaims struct {
Issuer string `json:"iss,omitempty"`
Subject string `json:"sub,omitempty"`
Audience jwt.Audience `json:"aud,omitempty"`
Expiry *jwt.NumericDate `json:"exp,omitempty"`
NotBefore *jwt.NumericDate `json:"nbf,omitempty"`
IssuedAt *jwt.NumericDate `json:"iat,omitempty"`
ID string `json:"jti,omitempty"`
Groups []string `json:"groups,omitempty"`
}
func Jsonify(v interface{}) (map[string]interface{}, error) {
data, err := json.Marshal(v)
if err != nil {
return nil, err
}
x := make(map[string]interface{})
return x, json.Unmarshal(data, &x)
}
func main() {
//type Claims struct {
// jwt.Claims
// Groups []string `json:"groups,omitempty"`
// Email string `json:"email,omitempty"`
// EmailVerified bool `json:"-"`
// Name string `json:"name,omitempty"`
/// ServiceAccountName string `json:"service_account_name,omitempty"`
// ServiceAccountNamespace string `json:"service_account_namespace,omitempty"`
// PreferredUsername string `json:"preferred_username,omitempty"`
// RawClaim map[string]interface{} `json:"-"`
// }
//claims := Claims{
// Claims: jwt.Claims{
// // Initialize fields of jwt.Claims as needed
// },
// Groups: []string{"abc", "ac1ef805-ac6a-4ce9-854a-1cb406aa7121"},
//}
type Claims struct {
CustomClaims
Email string `json:"email,omitempty"`
EmailVerified bool `json:"-"`
Name string `json:"name,omitempty"`
ServiceAccountName string `json:"service_account_name,omitempty"`
ServiceAccountNamespace string `json:"service_account_namespace,omitempty"`
PreferredUsername string `json:"preferred_username,omitempty"`
RawClaim map[string]interface{} `json:"-"`
}
claims := Claims{
CustomClaims: CustomClaims{
Groups: []string{"abc", "ac1ef805-ac6a-4ce9-854a-1cb406aa7121"},
},
}
//claims := Claims{Claims: jwt.Claims{Groups: []string{"abc", "ac1ef805-ac6a-4ce9-854a-1cb406aa7121"}}}
v, err := Jsonify(claims)
if err != nil {
panic(err)
}
input := `'ac1ef805-ac6a-4ce9-854a-1cb406aa7121' in groups
` //rule
program, err := expr.Compile(input, expr.Env(v))
if err != nil {
panic(err)
}
result, err := expr.Run(program, v)
if err != nil {
panic(err)
}
fmt.Println(result)
}
-- go.mod --
module play.ground
require github.com/expr-lang/expr v1.16.0 |
🤔 There were two separate users who encountered it though, and both resolved by downgrading. That would suggest that something in the diff between your code and Argo's is the buggy piece Also @tooptoop4 there's a "Share" button on the playground that's very helpful 😅 |
My intuition is that the |
Is it when That would add up since so far the reports have only been from users who were just starting to add SSO RBAC, and not from existing users of that feature (and would explain why more people haven't upvoted this as well) |
Can confirm in the playground, changing it to an empty array in the Groups: []string{},
There is no error, just prints
The Thanks @tooptoop4 for making a small repro in the playground for this to make it easier to investigate! |
|
Removing the |
Yea @antonmedv would it make sense for |
I think json tag can be used as expr tag via config like this:
What do you think? |
I tried debugging this and it seems the groups isnt part of the claims at all: you get the equivalent of:
|
I already root caused it above, and yes it will happen when you have no groups. |
In this case I believed I passed in groups from the IDP |
Sorry my bad, in my case i was missing the preceding '/' in the userInfoPath resulting in a 404 which was not being raised as an error, returning nil. If I am reading this correctly: https://github.com/argoproj/argo-workflows/blob/main/server/auth/types/claims.go#L102 Thanks for the patience though |
That's correct for that function, but it's used in |
I was able to resolve this on my side by explicitly declaring a group and ensuring that group was present in the claim sent back through our sso provider. I think either it should be updated to where when no groups match default role is applied, as the documentation suggests would happen - or the documentation updated to call out that the claim must match at least one group to the user even for the default role to apply. |
Totally agree. It will more convenient to fall back to default role... This might be optional, like fallback or forbid log in with the corresponding message. |
Pre-requisites
:latest
image tag (i.e.quay.io/argoproj/workflow-controller:latest
) and can confirm the issue still exists on:latest
. If not, I have explained why, in detail, in my description below.What happened/what you expected to happen?
Summary
All non-admin users assume a read-only role. After v3.5.5 is applied, non-admin users can no longer use argo-workflows and are prompted with a generic error in the UI:
Admin users can navigate normally.
Argo-server pod logs provide a bit more useful insight:
There aren't any additional logs that are related to this error that I can find.
Testing Performed
To confirm 3.5.5 introduced the issue I tested a few different versions with a read-only test user in a sandbox installation.
Configurations
Argo-Server
workflow-controller-configmap
Server Service Account
Server ClusterRoleBinding
Server ClusterRole
Additional Info
In a Slack convo Anton suggested that #12573 may be suspect.
Link to slack message thread
People engaged already:
Happy to provide any more information on request or connect to experiment/work through the issue
Version
v3.5.5
Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.
This isn't an issue with workflow execution.
Logs from the workflow controller
Logs from in your workflow's wait container
The text was updated successfully, but these errors were encountered: