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

feat: Set vars when TLS disabled or Dev enabled #2008

Closed
wants to merge 1 commit into from
Closed

feat: Set vars when TLS disabled or Dev enabled #2008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions cmd/gitops-server/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func NewCommand() *cobra.Command {
}

func runCmd(cmd *cobra.Command, args []string) error {
log, err := logger.New(options.LogLevel, options.Insecure)
log, err := logger.New(options.LogLevel, options.DevMode)
if err != nil {
return err
}
Expand Down Expand Up @@ -181,7 +181,8 @@ func runCmd(cmd *cobra.Command, args []string) error {
}

if options.DevMode {
log.Info("WARNING: dev mode enabled. This should be used for local work only")
log.Info("WARNING: dev mode enabled. Authentication will be bypassed in some instances. This should be used for LOCAL WORK ONLY.")
os.Setenv(server.DevModeFeatureFlag, "true")
tsv.SetDevMode(options.DevUser)
}

Expand Down Expand Up @@ -286,7 +287,10 @@ func runCmd(cmd *cobra.Command, args []string) error {

func listenAndServe(log logr.Logger, srv *http.Server, options Options) error {
if options.Insecure {
log.Info("TLS connections disabled")
log.Info(
"WARNING: TLS connections disabled by the `--insecure` flag. All data INCLUDING AUTH TOKENS will be transmitted without encryption.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this makes sense - we expect --insecure to pretty much always be set for security reasons: you'll have a separate ingress to encrypt the data, so you don't need to worry about how to configure allowed ciphers, special-case certificate renewal, and so on. That doesn't mean anything will be transmitted without encryption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh i didn't realised we did that now. In that case, why do we offer TLS at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looool excellent

i might be closing this one then

Copy link
Contributor

Choose a reason for hiding this comment

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

That other ingress is on the user to configure though right? And I assume we'd have no way of knowing whether they have appropriately configured? In which case a warning still feels valid but perhaps the wording changes to more of a "ensure you have an appropriately configured ingress..."

Copy link
Contributor

Choose a reason for hiding this comment

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

additionally, if we do indeed expect --insecure to be the default in a secure configuration... maybe we need to think about renaming :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #2010 to discuss what we really want to do around this

os.Setenv(server.TlsDisabledFeatureFlag, "true")

return srv.ListenAndServe()
}

Expand Down
10 changes: 9 additions & 1 deletion pkg/server/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ import (

const (
AuthEnabledFeatureFlag = "WEAVE_GITOPS_AUTH_ENABLED"
TlsDisabledFeatureFlag = "WEAVE_GITOPS_TLS_DISABLED"
DevModeFeatureFlag = "WEAVE_GITOPS_DEV_MODE_ENABLED"
ClusterUserAuthFlag = "CLUSTER_USER_AUTH"
OidcAuthFlag = "OIDC_AUTH"
)

var (
Expand All @@ -31,6 +35,10 @@ func AuthEnabled() bool {
return os.Getenv(AuthEnabledFeatureFlag) == "true"
}

func TlsEnabled() bool {
return os.Getenv(TlsDisabledFeatureFlag) != "true"
}

type Config struct {
AppConfig *ApplicationsConfig
AppOptions []ApplicationsOption
Expand All @@ -41,7 +49,7 @@ type Config struct {

func NewHandlers(ctx context.Context, log logr.Logger, cfg *Config) (http.Handler, error) {
mux := runtime.NewServeMux(middleware.WithGrpcErrorLogging(log))
httpHandler := middleware.WithLogging(log, mux)
httpHandler := middleware.WithLogging(log, mux, TlsEnabled())

if AuthEnabled() {
clustersFetcher, err := fetcher.NewSingleClusterFetcher(cfg.CoreServerConfig.RestCfg)
Expand Down
4 changes: 2 additions & 2 deletions pkg/server/middleware/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,15 @@ func WithGrpcErrorLogging(log logr.Logger) runtime.ServeMuxOption {

// WithLogging adds basic logging for HTTP requests.
// Note that this accepts a grpc-gateway ServeMux instead of an http.Handler.
func WithLogging(log logr.Logger, mux *runtime.ServeMux) http.Handler {
func WithLogging(log logr.Logger, mux *runtime.ServeMux, secure bool) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
recorder := &statusRecorder{
ResponseWriter: w,
Status: 200,
}
mux.ServeHTTP(recorder, r)

l := log.WithValues("uri", r.RequestURI, "status", recorder.Status)
l := log.WithValues("uri", r.RequestURI, "status", recorder.Status, "tls-secured", secure)

if recorder.Status < 400 {
l.V(logger.LogLevelDebug).Info(RequestOkText)
Expand Down
12 changes: 7 additions & 5 deletions pkg/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,9 @@ func (s *applicationServer) ValidateProviderToken(ctx context.Context, msg *pb.V
func (s *applicationServer) GetFeatureFlags(ctx context.Context, msg *pb.GetFeatureFlagsRequest) (*pb.GetFeatureFlagsResponse, error) {
flags := make(map[string]string)

flags["WEAVE_GITOPS_AUTH_ENABLED"] = os.Getenv("WEAVE_GITOPS_AUTH_ENABLED")
flags[AuthEnabledFeatureFlag] = os.Getenv(AuthEnabledFeatureFlag)
flags[TlsDisabledFeatureFlag] = os.Getenv(TlsDisabledFeatureFlag)
flags[DevModeFeatureFlag] = os.Getenv(DevModeFeatureFlag)

cl, err := s.clientGetter.Client(ctx)
if err != nil {
Expand All @@ -242,12 +244,12 @@ func (s *applicationServer) GetFeatureFlags(ctx context.Context, msg *pb.GetFeat

if err != nil {
if apierrors.IsNotFound(err) {
flags["CLUSTER_USER_AUTH"] = "false"
flags[ClusterUserAuthFlag] = "false"
} else {
s.log.Error(err, "could not get secret for cluster user")
}
} else {
flags["CLUSTER_USER_AUTH"] = "true"
flags[ClusterUserAuthFlag] = "true"
}

err = cl.Get(ctx, client.ObjectKey{
Expand All @@ -257,12 +259,12 @@ func (s *applicationServer) GetFeatureFlags(ctx context.Context, msg *pb.GetFeat

if err != nil {
if apierrors.IsNotFound(err) {
flags["OIDC_AUTH"] = "false"
flags[OidcAuthFlag] = "false"
} else {
s.log.Error(err, "could not get secret for oidc")
}
} else {
flags["OIDC_AUTH"] = "true"
flags[OidcAuthFlag] = "true"
}

return &pb.GetFeatureFlagsResponse{
Expand Down
110 changes: 79 additions & 31 deletions pkg/server/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ var _ = Describe("ApplicationsServer", func() {
fakeClientGetter := kubefakes.NewFakeClientGetter(k8s)
appsSrv := server.NewApplicationsServer(&cfg, server.WithClientGetter(fakeClientGetter))
mux = runtime.NewServeMux(middleware.WithGrpcErrorLogging(log))
httpHandler := middleware.WithLogging(log, mux)
httpHandler := middleware.WithLogging(log, mux, true)
err := pb.RegisterApplicationsHandlerServer(context.Background(), mux, appsSrv)
Expect(err).NotTo(HaveOccurred())

Expand Down Expand Up @@ -458,10 +458,6 @@ func contextWithAuth(ctx context.Context) context.Context {
}

func TestGetFeatureFlags(t *testing.T) {
type Data struct {
Flags map[string]string
}

tests := []struct {
name string
envSet func()
Expand All @@ -472,31 +468,35 @@ func TestGetFeatureFlags(t *testing.T) {
{
name: "Auth enabled",
envSet: func() {
os.Setenv("WEAVE_GITOPS_AUTH_ENABLED", "true")
os.Setenv(server.AuthEnabledFeatureFlag, "true")
},
envUnset: func() {
os.Unsetenv("WEAVE_GITOPS_AUTH_ENABLED")
os.Unsetenv(server.AuthEnabledFeatureFlag)
},
state: []client.Object{},
result: map[string]string{
"WEAVE_GITOPS_AUTH_ENABLED": "true",
"CLUSTER_USER_AUTH": "false",
"OIDC_AUTH": "false",
server.AuthEnabledFeatureFlag: "true",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "",
},
},
{
name: "Auth disabled",
envSet: func() {
os.Setenv("WEAVE_GITOPS_AUTH_ENABLED", "false")
os.Setenv(server.AuthEnabledFeatureFlag, "false")
},
envUnset: func() {
os.Unsetenv("WEAVE_GITOPS_AUTH_ENABLED")
os.Unsetenv(server.AuthEnabledFeatureFlag)
},
state: []client.Object{},
result: map[string]string{
"WEAVE_GITOPS_AUTH_ENABLED": "false",
"CLUSTER_USER_AUTH": "false",
"OIDC_AUTH": "false",
server.AuthEnabledFeatureFlag: "false",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "",
},
},
{
Expand All @@ -505,9 +505,11 @@ func TestGetFeatureFlags(t *testing.T) {
envUnset: func() {},
state: []client.Object{},
result: map[string]string{
"WEAVE_GITOPS_AUTH_ENABLED": "",
"CLUSTER_USER_AUTH": "false",
"OIDC_AUTH": "false",
server.AuthEnabledFeatureFlag: "",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "",
},
},
{
Expand All @@ -516,9 +518,11 @@ func TestGetFeatureFlags(t *testing.T) {
envUnset: func() {},
state: []client.Object{&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: "flux-system", Name: "cluster-user-auth"}}},
result: map[string]string{
"WEAVE_GITOPS_AUTH_ENABLED": "",
"CLUSTER_USER_AUTH": "true",
"OIDC_AUTH": "false",
server.AuthEnabledFeatureFlag: "",
server.ClusterUserAuthFlag: "true",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "",
},
},
{
Expand All @@ -527,9 +531,11 @@ func TestGetFeatureFlags(t *testing.T) {
envUnset: func() {},
state: []client.Object{},
result: map[string]string{
"WEAVE_GITOPS_AUTH_ENABLED": "",
"CLUSTER_USER_AUTH": "false",
"OIDC_AUTH": "false",
server.AuthEnabledFeatureFlag: "",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "",
},
},
{
Expand All @@ -538,9 +544,11 @@ func TestGetFeatureFlags(t *testing.T) {
envUnset: func() {},
state: []client.Object{&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: "flux-system", Name: "oidc-auth"}}},
result: map[string]string{
"WEAVE_GITOPS_AUTH_ENABLED": "",
"CLUSTER_USER_AUTH": "false",
"OIDC_AUTH": "true",
server.AuthEnabledFeatureFlag: "",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "true",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "",
},
},
{
Expand All @@ -549,11 +557,51 @@ func TestGetFeatureFlags(t *testing.T) {
envUnset: func() {},
state: []client.Object{},
result: map[string]string{
"WEAVE_GITOPS_AUTH_ENABLED": "",
"CLUSTER_USER_AUTH": "false",
"OIDC_AUTH": "false",
server.AuthEnabledFeatureFlag: "",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "",
},
},
{
name: "TLS disabled",
envSet: func() {
os.Setenv(server.TlsDisabledFeatureFlag, "true")
},
envUnset: func() {
os.Unsetenv(server.TlsDisabledFeatureFlag)
},
state: []client.Object{},
result: map[string]string{
server.AuthEnabledFeatureFlag: "",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "true",
server.DevModeFeatureFlag: "",
},
},
{
name: "dev mode enabled",
envSet: func() {
os.Setenv(server.DevModeFeatureFlag, "true")
},
envUnset: func() {
os.Unsetenv(server.DevModeFeatureFlag)
},
state: []client.Object{},
result: map[string]string{
server.AuthEnabledFeatureFlag: "",
server.ClusterUserAuthFlag: "false",
server.OidcAuthFlag: "false",
server.TlsDisabledFeatureFlag: "",
server.DevModeFeatureFlag: "true",
},
},
}

type Data struct {
Flags map[string]string
}

for _, tt := range tests {
Expand All @@ -571,7 +619,7 @@ func TestGetFeatureFlags(t *testing.T) {
err := pb.RegisterApplicationsHandlerServer(context.Background(), mux, appSrv)
Expect(err).NotTo(HaveOccurred())

httpHandler := middleware.WithLogging(log, mux)
httpHandler := middleware.WithLogging(log, mux, true)

ts := httptest.NewServer(httpHandler)
defer ts.Close()
Expand Down