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.

Sign up for GitHub

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

Jump to bottom

chore(oidc): Add log client IP address #20901

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+20 −12
Open

chore(oidc): Add log client IP address #20901

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5620378
oidc: Add log client IP address
omerap12 Nov 21, 2024
c26c453
Merge branch 'master' into issue-20388
omerap12 Nov 22, 2024
d7c10b1
Refactor OIDC request metadata creation
omerap12 Nov 22, 2024
4cd2228
Refactor OIDC logging and error handling
omerap12 Nov 22, 2024
f8fdd89
Fix logger error messages in oidc.go
omerap12 Nov 22, 2024
42d878a
Remove unused logger variable in HandleLogin function
omerap12 Nov 22, 2024
28074ec
Add logger to HandleLogin and HandleCallback functions
omerap12 Nov 22, 2024
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
24 changes: 21 additions & 3 deletions util/oidc/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,21 @@ const (
AccessTokenCachePrefix = "access_token"
)

// Request Metadata for warning message
type RequestMetadata struct {
ipAddr string
}

func (r *RequestMetadata) String() string {
return fmt.Sprintf("Client IP: %s", r.ipAddr)
}

func newRequestMetadata(r *http.Request) *RequestMetadata {
return &RequestMetadata{
ipAddr: r.RemoteAddr,
}
}

omerap12 marked this conversation as resolved.
Show resolved Hide resolved
// OIDCConfiguration holds a subset of interested fields from the OIDC configuration spec
type OIDCConfiguration struct {
Issuer string `json:"issuer"`
Expand Down Expand Up @@ -149,13 +164,14 @@ func NewClientApp(settings *settings.ArgoCDSettings, dexServerAddr string, dexTl
}

func (a *ClientApp) oauth2Config(request *http.Request, scopes []string) (*oauth2.Config, error) {
rMetadata := newRequestMetadata(request)
endpoint, err := a.provider.Endpoint()
if err != nil {
return nil, err
}
redirectURL, err := a.settings.RedirectURLForRequest(request)
if err != nil {
log.Warnf("Unable to find ArgoCD URL from request, falling back to configured redirect URI: %v", err)
log.Warnf("Unable to find ArgoCD URL from request, falling back to configured redirect URI: %v. %s", err, rMetadata)
redirectURL = a.redirectURI
}
return &oauth2.Config{
Expand Down Expand Up @@ -197,6 +213,7 @@ func (a *ClientApp) generateAppState(returnURL string, w http.ResponseWriter) (s
}

func (a *ClientApp) verifyAppState(r *http.Request, w http.ResponseWriter, state string) (string, error) {
rMetadata := newRequestMetadata(r)
c, err := r.Cookie(common.StateCookieName)
if err != nil {
return "", err
Expand All @@ -219,7 +236,7 @@ func (a *ClientApp) verifyAppState(r *http.Request, w http.ResponseWriter, state
if len(sanitizedUrl) > 100 {
sanitizedUrl = sanitizedUrl[:100]
}
log.Warnf("Failed to verify app state - got invalid redirectURL %q", sanitizedUrl)
log.Warnf("Failed to verify app state - got invalid redirectURL %q. %s", sanitizedUrl, rMetadata)
return "", fmt.Errorf("failed to verify app state: %w", InvalidRedirectURLError)
}
redirectURL = parts[1]
Expand Down Expand Up @@ -338,6 +355,7 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) {

// HandleCallback is the callback handler for an OAuth2 login flow
func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {
rMetadata := newRequestMetadata(r)
oauth2Config, err := a.oauth2Config(r, nil)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
Expand Down Expand Up @@ -375,7 +393,7 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) {

idToken, err := a.provider.Verify(idTokenRAW, a.settings)
if err != nil {
log.Warnf("Failed to verify token: %s", err)
log.Warnf("Failed to verify token: %s. %s", err, rMetadata)
http.Error(w, common.TokenVerificationError, http.StatusInternalServerError)
return
}
Expand Down
3 changes: 0 additions & 3 deletions util/oidc/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,6 @@ func (p *providerImpl) Verify(tokenString string, argoSettings *settings.ArgoCDS
if err == nil {
break
}
// We store the error for each audience so that we can return a more detailed error message to the user.
// If this gets merged, we'll be able to detect failures unrelated to audiences and short-circuit this loop
// to avoid logging irrelevant warnings: https://github.com/coreos/go-oidc/pull/406
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue specifies this comment should be removed

tokenVerificationErrors[aud] = err
}
// If the most recent attempt encountered an error, and if we have collected multiple errors, switch to the
Expand Down
Loading