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

Conditionally Enable filters.WithAuthenticationAndAuthorization Only When secureMetrics is True #4020

Closed
camilamacedo86 opened this issue Jul 17, 2024 · 6 comments · Fixed by #4022
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-blocker

Comments

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jul 17, 2024

What do you want to happen?

In the default scaffold of main.go, the FilterProvider: filters.WithAuthenticationAndAuthorization is always enabled. However, this can lead to security issues as it requires a ServiceAccount token to always be provided, which should never be sent via HTTP.

Problem Statement:

By always enabling filters.WithAuthenticationAndAuthorization, users are forced to provide a ServiceAccount token, even in cases where secureMetrics is not set to true. This can lead to tokens being sent over insecure connections, which is not advisable.

Proposal:

The filters.WithAuthenticationAndAuthorization should only be enabled when secureMetrics is set to true. This will ensure that ServiceAccount tokens are only required and used when secure communication is enabled.
Update the implementation in the default scaffold of main.go to conditionally enable filters.WithAuthenticationAndAuthorization based on the value of secureMetrics.

References:

Expected Behavior:

The filters.WithAuthenticationAndAuthorization should be conditional and only enabled when secureMetrics is true. This ensures that ServiceAccount tokens are only needed in a secure environment, preventing them from being sent over HTTP.

Proposed Solution:

Example Code Change:

Metrics: metricsserver.Options {
    BindAddress:   metricsAddr,
    SecureServing: secureMetrics,
    TLSOpts:       tlsOpts,
    FilterProvider: func() metricsserver.FilterProvider {
        if secureMetrics {
            return filters.WithAuthenticationAndAuthorization
        }
        return nil
    }(),
},

Extra Labels

No response

@camilamacedo86 camilamacedo86 added kind/feature Categorizes issue or PR as related to a new feature. kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-blocker and removed kind/feature Categorizes issue or PR as related to a new feature. labels Jul 17, 2024
@camilamacedo86
Copy link
Member Author

@sbueringer,

Thank you for raise this one.

@camilamacedo86 camilamacedo86 added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Jul 17, 2024
@sbueringer
Copy link
Member

sbueringer commented Jul 17, 2024

I think I would prefer only setting a FilterProvider if secureMetrics is true (compared to setting one that returns nil).

But I think both works today with controller-runtime.

I would generally expect that if a FilterProvider is set it either returns an error or it returns a FilterProvider, not nil, nil

@alex-kattathra-johnson
Copy link
Contributor

alex-kattathra-johnson commented Jul 17, 2024

@sbueringer: Looks like the example code is setting the FilterProvider only if secureMetrics is true, the anonymous function returns a FilterProvider or nil based on that condition. I'd be interested in picking this up as a first issue! :)

@sbueringer
Copy link
Member

Ups sorry. You're right!

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Jul 17, 2024

Hi @sbueringer and @alex-kattathra-johnson

The example code is only to show where/what we need to do.
Anyway that does the achieve the expected result it is fine, it can either to be something like:

Metrics: metricsserver.Options {
    BindAddress:   metricsAddr,
    SecureServing: secureMetrics,
    TLSOpts:       tlsOpts,
    if secureMetrics {
         FilterProvider: func() metricsserver.FilterProvider {
            return filters.WithAuthenticationAndAuthorization
        }   
    },
},

I would either prefer something simple like above 👍

@sbueringer
Copy link
Member

sbueringer commented Jul 17, 2024

All good, I was just misreading the anonymous func :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-blocker
Projects
None yet
3 participants