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

fix insecure configurations #3235

Closed
lel2021 opened this issue Jan 27, 2024 · 3 comments
Closed

fix insecure configurations #3235

lel2021 opened this issue Jan 27, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@lel2021
Copy link

lel2021 commented Jan 27, 2024

What steps did you take and what happened:
[A clear and concise description of what the bug is.]
third_party/sigs.k8s.io/controller-runtime/pkg/webhook/server.go

// StartedChecker returns an healthz.Checker which is healthy after the
// server has been started.
func (s *DefaultServer) StartedChecker() healthz.Checker {
	config := &tls.Config{
		InsecureSkipVerify: true, //nolint:gosec // config is used to connect to our own webhook port.
	}
	return func(req *http.Request) error {
		s.mu.Lock()
		defer s.mu.Unlock()

		if !s.started {
			return fmt.Errorf("webhook server has not been started yet")
		}

		d := &net.Dialer{Timeout: 10 * time.Second}
		conn, err := tls.DialWithDialer(d, "tcp", net.JoinHostPort(s.Options.Host, strconv.Itoa(s.Options.Port)), config)
		if err != nil {
			return fmt.Errorf("webhook server is not reachable: %w", err)
		}

		if err := conn.Close(); err != nil {
			return fmt.Errorf("webhook server is not reachable: closing connection: %w", err)
		}

		return nil
	}
}

``
The client uses an insecure configuration of InsecureSkipVerify, and not set MinVersion to VersionTLS12

What did you expect to happen:
use secure tls configurations

tls.Config{
		InsecureSkipVerify: false,
                MinVersion: VersionTLS12,
	}

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

  • Gatekeeper version: 3.13.0
  • Kubernetes version: 1.27:
@lel2021 lel2021 added the bug Something isn't working label Jan 27, 2024
@maxsmythe
Copy link
Contributor

maxsmythe commented Feb 3, 2024

It looks like this is in the controller-runtime code base? It would be better to file a bug with them if this is an issue, so the fix can roll out to multiple projects.

That being said, it looks like this checker is only used to determine liveness of the server. In a default config, host is "", which means the dial command only reaches out to localhost, which removes the risk of network hops. I could maybe see an attack vector if a different host was used (pretending the webhook is serving to increase the likelihood of failed requests), but the inverse attack is also possible with TLS (black-hole the requests, so all webhooks are perma-down).

Ultimately, I think using s.Options.Host is probably a mistake, since it may cause the liveliness check to go to the wrong server if host is a non-empty string. I'd ask controller-runtime to consider removing that.

Here is the flag where Gatekeeper defaults host to empty string:

host = flag.String("host", "", "the host address the webhook server listens on. defaults to all addresses.")

@maxsmythe
Copy link
Contributor

Then again, I suppose if a user DID use host, then ignoring that may cause the Dial to fail -- ultimately host should always be a local IP (if not, how would the request be served? If it's not a local IP, the host will not receive the request, if it is a hostname, golang does not recommend that for functional issues):

The address can use a host name, but this is not recommended, because it will create a listener for at most one of the host's IP addresses

https://pkg.go.dev/net#Listen

I think this bug may be moot.

@sozercan
Copy link
Member

no longer applicable since fork is removed with #3111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants