Skip to content

Commit

Permalink
NVSHAS-7616: fix a few warnings and cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@holyspectral
holyspectral committed Nov 10, 2023
1 parent 4731d94 commit f328173
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 61 deletions.
37 changes: 20 additions & 17 deletions controller/api/apis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,10 @@ type RESTFedAuthData struct {
type RESTTokenRedirect struct {
// The NeuVector URL to redirect after authentication/logout.
Redirect string `json:"redirect_endpoint"`
// Optional: The redirect url is used as issuer by default. When isser is specified, it will be used instead of redirecturl.
// (Optional)
// When absent, the redirect url will be used as issuer in SAML request.
// When it is specified, the value here will be used as the issuer.
// This is for Single Logout where redirect url and issue can be different.
Issuer string `json:"issuer"`
}

Expand Down Expand Up @@ -351,11 +354,11 @@ type RESTServerSAML struct {
RoleGroups map[string][]string `json:"role_groups,omitempty"` // role -> groups
GroupMappedRoles []*share.GroupRoleMapping `json:"group_mapped_roles,omitempty"` // group -> (role -> domains)

AuthnSigningEnabled bool `json:"authn_signing_enabled,omitempty"`
SigningCert string `json:"signing_cert,omitempty"`
SigningKey string `json:"signing_key,omitempty"`
SLOEnabled bool `json:"slo_enabled,omitempty"`
SLOURL string `json:"slo_url,omitempty"`
AuthnSigningEnabled bool `json:"authn_signing_enabled,omitempty"` // Optional. Enable signing AuthnRequest. Default off.
SigningCert string `json:"signing_cert,omitempty"` // Optional.
//SigningKey string `json:"signing_key,omitempty"` // Optional.
SLOEnabled bool `json:"slo_enabled,omitempty"` // Optional.
SLOURL string `json:"slo_url,omitempty"` // Optional.
}

type RESTServerOIDC struct {
Expand Down Expand Up @@ -431,11 +434,11 @@ type RESTServerSAMLConfig struct {
GroupMappedRoles *[]*share.GroupRoleMapping `json:"group_mapped_roles,omitempty"` // group -> (role -> domains)
X509CertExtra *[]string `json:"x509_cert_extra,omitempty"`

AuthnSigningEnabled *bool `json:"authn_signing_enabled,omitempty"` // Optional. Enable signing AuthnRequest.
AuthnSigningEnabled *bool `json:"authn_signing_enabled,omitempty"` // Optional. Enable signing AuthnRequest. Default off.
SigningCert *string `json:"signing_cert,omitempty"` // Optional.
SigningKey *string `json:"signing_key,omitempty"` // Optional.
SLOEnabled *bool `json:"slo_enabled,omitempty"` // Optional.
SLOURL *string `json:"slo_url,omitempty"` // Optional.
SigningCert *string `json:"signing_cert,omitempty"` // Optional. Used as signing cert by default.
SigningKey *string `json:"signing_key,omitempty"` // Optional. Used as signing key by default.
}

type RESTServerSAMLConfigCfgMap struct {
Expand Down Expand Up @@ -1059,14 +1062,14 @@ type RESTConversationEndpointConfigData struct {
}

type RESTConversationReportEntry struct {
Bytes uint64 `json:"bytes"`
Sessions uint32 `json:"sessions"`
Port string `json:"port,omitempty"`
Application string `json:"application,omitempty"`
PolicyAction string `json:"policy_action"`
CIP string `json:"client_ip,omitempty"`
SIP string `json:"server_ip,omitempty"`
FQDN string `json:"fqdn,omitempty"`
Bytes uint64 `json:"bytes"`
Sessions uint32 `json:"sessions"`
Port string `json:"port,omitempty"`
Application string `json:"application,omitempty"`
PolicyAction string `json:"policy_action"`
CIP string `json:"client_ip,omitempty"`
SIP string `json:"server_ip,omitempty"`
FQDN string `json:"fqdn,omitempty"`
}

type RESTConversationReport struct {
Expand Down
12 changes: 6 additions & 6 deletions controller/kv/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
"github.com/neuvector/neuvector/controller/access"
"github.com/neuvector/neuvector/controller/api"
"github.com/neuvector/neuvector/controller/common"
admission "github.com/neuvector/neuvector/controller/nvk8sapi/nvvalidatewebhookcfg"
"github.com/neuvector/neuvector/controller/nvk8sapi/nvvalidatewebhookcfg"
"github.com/neuvector/neuvector/controller/resource"
"github.com/neuvector/neuvector/share"
"github.com/neuvector/neuvector/share/cluster"
Expand Down Expand Up @@ -485,11 +485,11 @@ func (c *configHelper) sections2Endpoints(sections []string) []*cfgEndpoint {
return eps
}

// 1. When import, cluster name is always replaced with the cluster name(if available) specified in the backup file
// 2. When import, fed rules are always replaced with the fed rules specified in the backup file.
// 3. For clusters in fed, Import() doesn't change the existing clusters membership.
// 4. For stand-alone cluster, we allow it to promote to master cluster by importing a master cluster's backup file.
// However, joined clusters list is not imported. Customer needs to manually trigger join-fed operation.
// 1. When import, cluster name is always replaced with the cluster name(if available) specified in the backup file
// 2. When import, fed rules are always replaced with the fed rules specified in the backup file.
// 3. For clusters in fed, Import() doesn't change the existing clusters membership.
// 4. For stand-alone cluster, we allow it to promote to master cluster by importing a master cluster's backup file.
// However, joined clusters list is not imported. Customer needs to manually trigger join-fed operation.
func (c *configHelper) Import(rpcEps []*common.RPCEndpoint, localCtrlerID, localCtrlerIP string, loginDomainRoles access.DomainRole, importTask share.CLUSImportTask,
tempToken string, revertFedRoles RevertFedRolesFunc, postImportOp PostImportFunc, pauseResumeStoreWatcher PauseResumeStoreWatcherFunc, ignoreFed bool) error {
log.Debug()
Expand Down
2 changes: 1 addition & 1 deletion controller/rest/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2609,7 +2609,7 @@ func handlerAuthLogout(w http.ResponseWriter, r *http.Request, ps httprouter.Par
fedRole, err := cacher.GetFedMembershipRole(acc)
if err == nil && fedRole == api.FedRoleMaster {
ids := cacher.GetFedJoinedClusterIdMap(acc)
for id, _ := range ids {
for id := range ids {
joinedCluster := cacher.GetFedJoinedCluster(id, acc)
if joinedCluster.ID != "" {
var token string
Expand Down
8 changes: 4 additions & 4 deletions controller/rest/rest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1446,10 +1446,10 @@ func StartRESTServer() {
//r.POST("/v1/password_profile", handlerPwdProfileCreate)
r.PATCH("/v1/password_profile/:name", handlerPwdProfileConfig)
//r.DELETE("/v1/password_profile/:name", handlerPwdProfileDelete)
r.GET("/v1/token_auth_server", handlerTokenAuthServerList) // Skip API document
r.GET("/v1/token_auth_server/:server", handlerTokenAuthServerRequest) // Skip API document
r.POST("/v1/token_auth_server/:server", handlerTokenAuthServerRequest) // Skip API document
r.GET("/v1/token_auth_server/:server/slo", handlerGenerateSLORequest) // Skip API document
r.GET("/v1/token_auth_server", handlerTokenAuthServerList) // Skip API document
r.GET("/v1/token_auth_server/:server", handlerTokenAuthServerRequest)
r.POST("/v1/token_auth_server/:server", handlerTokenAuthServerRequest)
r.GET("/v1/token_auth_server/:server/slo", handlerGenerateSLORequest)
r.GET("/v1/server", handlerServerList)
r.GET("/v1/server/:name", handlerServerShow)
r.GET("/v1/server/:name/user", handlerServerUserList)
Expand Down
13 changes: 10 additions & 3 deletions controller/rest/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ import (
"crypto/x509"
"encoding/json"
"encoding/pem"
"errors"
"fmt"
"io/ioutil"
"net/http"
"net/url"
"sort"
"strings"

"github.com/pkg/errors"

"github.com/julienschmidt/httprouter"
log "github.com/sirupsen/logrus"

Expand Down Expand Up @@ -451,6 +452,7 @@ func handlerGenerateSLORequest(w http.ResponseWriter, r *http.Request, ps httpro

// Handle SAML SLO
if login.nameid == "" {
log.Debug("This user has no nameid associated. Do not generate SAML SLO request.")
restRespSuccess(w, r, &resp, acc, login, nil, "")
return
}
Expand All @@ -462,6 +464,7 @@ func handlerGenerateSLORequest(w http.ResponseWriter, r *http.Request, ps httpro
}

if !cs.SAML.SLOEnabled {
log.Debug("SAML SLO is not enabled.")
restRespSuccess(w, r, &resp, acc, login, nil, "")
return
}
Expand Down Expand Up @@ -595,7 +598,7 @@ func validateAuthServer(cas *share.CLUSServerAuth) error {
if !access.IsValidRole(mappedRoles.GlobalRole, access.CONST_VISIBLE_USER_ROLE) {
return fmt.Errorf("Invalid global role(%s) in group(%s) mapping", mappedRoles.GlobalRole, mappedRoles.Group)
}
for role, _ := range mappedRoles.RoleDomains {
for role := range mappedRoles.RoleDomains {
if !access.IsValidRole(role, access.CONST_VISIBLE_DOMAIN_ROLE) {
return fmt.Errorf("Invalid domain role(%s) in group(%s) mapping", role, mappedRoles.Group)
}
Expand Down Expand Up @@ -745,7 +748,11 @@ func validateSAMLServer(cs *share.CLUSServer) error {
}
}

// TODO: Verify encryption key/certs
if len(csaml.SigningCert) > 0 || len(csaml.SigningKey) > 0 {
if _, err := tls.X509KeyPair([]byte(csaml.SigningCert), []byte(csaml.SigningKey)); err != nil {
return errors.Wrap(err, "invalid key cert pair")
}
}

var certs []string
certs = append(certs, csaml.X509Cert) // original one
Expand Down
29 changes: 12 additions & 17 deletions share/auth/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,30 +144,29 @@ func GenerateSamlSP(csaml *share.CLUSServerSAML, spissuer string, redirurl strin
Roots: []*x509.Certificate{},
}

// TODO: fix error handling
parseAndStoreCert := func(x509cert string) {
parseAndStoreCert := func(x509cert string) error {
var err error
defer func() {
if err != nil {
log.WithError(err).Warn("failed to decode cert.")
}
}()
block, _ := pem.Decode([]byte(x509cert))
if block == nil {
err = errors.New("failed to decode pem block")
return errors.New("failed to decode pem block")
}

idpCert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
return
return err
}
certStore.Roots = append(certStore.Roots, idpCert)
return nil
}

parseAndStoreCert(csaml.X509Cert)
if err := parseAndStoreCert(csaml.X509Cert); err != nil {
log.WithError(err).Error("failed to parse X509Cert. Skip this cert.")
}

for _, cert := range csaml.X509CertExtra {
parseAndStoreCert(cert)
if err := parseAndStoreCert(cert); err != nil {
log.WithError(err).Error("failed to parse X509Cert. Skip this cert.")
}
}

if csaml.SigningCert != "" && csaml.SigningKey != "" {
Expand All @@ -178,6 +177,7 @@ func GenerateSamlSP(csaml *share.CLUSServerSAML, spissuer string, redirurl strin
keystore = dsig.TLSCertKeyStore(cert)
}

// For unit-test
var clockOverride *dsig.Clock
if timeOverride != nil {
clockOverride = dsig.NewFakeClock(clockwork.NewFakeClockAt(*timeOverride))
Expand Down Expand Up @@ -275,13 +275,8 @@ func (a *remoteAuth) SAMLSPGetLogoutURL(csaml *share.CLUSServerSAML, redir *api.
}

// Return Name ID, session index, and attributes.
// TODO: Check all certs
func (a *remoteAuth) SAMLSPAuth(csaml *share.CLUSServerSAML, tokenData *api.RESTAuthToken) (string, string, map[string][]string, error) {
var certs []string
certs = append(certs, csaml.X509Cert)
certs = append(certs, csaml.X509CertExtra...)

// For backward compatibility, use Authn response redirect url (AssertionConsumerServiceURL) as SP issuer. (https://<NV>/token_auth_server)
// Authn response redirect url (AssertionConsumerServiceURL) as SP issuer. (https://<NV>/token_auth_server)
sp, err := GenerateSamlSP(csaml, tokenData.Redirect, tokenData.Redirect, a.fakeTime)
if err != nil {
return "", "", map[string][]string{}, err
Expand Down
26 changes: 13 additions & 13 deletions share/auth/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,20 +151,20 @@ ma7nkie3ORja96UTROAZ77o=
},
},
},
SSOURL: "https://dev.okta.com/app/dev/xxxxx/sso/saml",
Issuer: "https://example.com/token_auth_server",
X509Cert: string(idpCert),
SLOEnabled: false,
SLOSigningCert: string(cert),
SLOSigningKey: string(key),
SSOURL: "https://dev.okta.com/app/dev/xxxxx/sso/saml",
Issuer: "https://example.com/token_auth_server",
X509Cert: string(idpCert),
SLOEnabled: false,
SigningCert: string(cert),
SigningKey: string(key),
}, &reqData,
map[string]string{
"ID": "_ec47ec78-a7f4-458d-86ad-f8e5dc85eb8a",
},
)

// Verify if it's consistent with Authn request for Okta.
assert.Equal(t, "https://dev.okta.com/app/dev/xxxxx/sso/saml?SAMLRequest=jJJBb9swDIX%2FisC7LcdNFkOoA2QNhgXotqDJdtglYGVmEWpJnkhl2b8f4rZAd1gwHSk%2Bfg98vGX0%2FWCWWY7hgX5mYlFn3wc240cLOQUTkR2bgJ7YiDXb5ad7U5eVGVKUaGMPbyTXFchMSVwMoNarFvZkp3Oy86bA%2BWFaTGdNVzTvsCsODc0628zosUFQ3yixi6GFuqxAbV6o713oXPhxHfj43MTm4263KTZftjtQy1cTdzFw9pS2lE7O0teH%2BxaOIgMbremMfuiptNFriU8U9pjluGdKJ0qg1syZ1oEFg7RQV%2FVNMamKarqrJ6ZqzE31HdSKWFxAGa2%2Fzu3oVMYnwXEwDsOloM%2BXp5mjvqwQFmMqZmSkxX85utVvJS%2BpfkZP69Um9s7%2BVsu%2Bj7%2FuEqFQC5IygfoQk0f59wIn5WSsuK44jK0mBx7IuoOjDvTiGfr39Sz%2BBAAA%2F%2F8%3D&SigAlg=http%3A%2F%2Fwww.w3.org%2F2001%2F04%2Fxmldsig-more%23rsa-sha256&Signature=MMolrRAjT%2BaWEuh8erz%2B280%2B8pdgAs%2BCeJVEGgDXWo6cLDTjkTCgpP9SkzBpocW6IbjSG9eYcYB0SBTk47vW71Jv16Kq6eVQc6bNMYQVbiXwXEV6ZY0HjLaqeu%2FS0oiE6j6z9SJBSp2U22dYxBhSjrfK6uIorLFvG1mul7XTUTsa5WXQeEF90%2Bu3%2Bh8wJ3GONuWzr6rLOimVbJnqUHFQA8MJLQNWVlIlnv64Me%2FUU25zLSOZ6SKx7%2FxWsAYymoUIOt8%2Ft%2FfFhkulPbxZxVAoLwzjSiJAkLEFIwaRkiRWTZ6Xwi6h7jW37OGN4GjVE25S0fRxR1fMh88u3XRJ0Tr2zw%3D%3D", url)
assert.Equal(t, "https://dev.okta.com/app/dev/xxxxx/sso/saml?SAMLRequest=jJJBb9swDIX%2FisC7LcdNFkOoA2QNhgXotqDJdtglYGVmEWpJnkhl2b8f4rZAd1gwHSk%2Bfg98vGX0%2FWCWWY7hgX5mYlFn3wc240cLOQUTkR2bgJ7YiDXb5ad7U5eVGVKUaGMPbyTXFchMSVwMoNarFvZkp3Oy86bA%2BWFaTGdNVzTvsCsODc0628zosUFQ3yixi6GFuqxAbV6o713oXPhxHfj43MTm4263KTZftjtQy1cTdzFw9pS2lE7O0teH%2BxaOIgMbremMfuiptNFriU8U9pjluGdKJ0qg1syZ1oEFg7RQV%2FVNMamKarqrJ6ZqzE31HdSKWFxAGa2%2Fzu3oVMYnwXEwDsOloM%2BXp5mjvqwQFmMqZmSkxX85utVvJS%2BpfkZP69Um9s7%2BVsu%2Bj7%2FuEqFQC5IygfoQk0f59wIn5WSsuK44jK0mBx7IuoOjDvTiGfr39Sz%2BBAAA%2F%2F8%3D", url)
}

// Verify NV can accept Okta Authn response.
Expand Down Expand Up @@ -305,12 +305,12 @@ ma7nkie3ORja96UTROAZ77o=
},
SSOURL: "https://dev.okta.com/app/dev/xxxxx/sso/saml",
// TODO: Make sure issuer and redirect don't conflict.
Issuer: "https://example.com/token_auth_server",
X509Cert: string(idpCert),
SLOEnabled: true,
SLOURL: "https://dev.okta.com/app/dev/xxxxx/slo/saml",
SLOSigningCert: string(cert),
SLOSigningKey: string(key),
Issuer: "https://example.com/token_auth_server",
X509Cert: string(idpCert),
SLOEnabled: true,
SLOURL: "https://dev.okta.com/app/dev/xxxxx/slo/saml",
SigningCert: string(cert),
SigningKey: string(key),
}, &reqData,
"[email protected]",
"_6d713693-660a-4740-b9e0-c1ac2321fabe",
Expand Down

0 comments on commit f328173

Please sign in to comment.