[CLD-6304] IP Validator for IP Filtering + Storage Changes for UX (#970)

* Add IP validator to IP filtering feature. Fix a minor bug in override mechanism Signed-off-by: Stavros Foteinopoulos <[email protected]> * lint fixes Signed-off-by: Stavros Foteinopoulos <[email protected]> * review fixes Signed-off-by: Stavros Foteinopoulos <[email protected]> * Adjust support of IP Filtering storage to be JSON rather than a CSV list of CIDR blocks * Update the merge logic, add a test for it * Fix tests * Fix tests --------- Signed-off-by: Stavros Foteinopoulos <[email protected]> Co-authored-by: Stavros Foteinopoulos <[email protected]>
mattermost · Oct 5, 2023 · dab0959 · dab0959
1 parent c6344ec
commit dab0959
Show file tree

Hide file tree

Showing 7 changed files with 495 additions and 50 deletions.
diff --git a/cmd/cloud/installation_flag.go b/cmd/cloud/installation_flag.go
@@ -116,7 +116,7 @@ func (flags *installationPatchRequestOptions) addFlags(command *cobra.Command) {
 	command.Flags().StringVar(&flags.image, "image", "mattermost/mattermost-enterprise-edition", "The Mattermost container image to use.")
 	command.Flags().StringVar(&flags.size, "size", model.InstallationDefaultSize, "The size of the installation. Accepts 100users, 1000users, 5000users, 10000users, 25000users, miniSingleton, or miniHA. Defaults to 100users.")
 	command.Flags().StringVar(&flags.license, "license", "", "The Mattermost License to use in the server.")
-	command.Flags().StringVar(&flags.allowedIPRanges, "allowed-ip-ranges", "", "The IP Ranges that is allowed the workspace to be accessed from.")
+	command.Flags().StringVar(&flags.allowedIPRanges, "allowed-ip-ranges", "", "JSON Encoded list of IP Ranges that are allowed to access the workspace.")
 	command.Flags().StringArrayVar(&flags.mattermostEnv, "mattermost-env", []string{}, "Env vars to add to the Mattermost App. Accepts format: KEY_NAME=VALUE. Use the flag multiple times to set multiple env vars.")
 	command.Flags().BoolVar(&flags.mattermostEnvClear, "mattermost-env-clear", false, "Clears all env var data.")
 	command.Flags().BoolVar(&flags.overrideIPRanges, "override-ip-ranges", true, "Overrides Allowed IP ranges and force ignoring any previous value.")
@@ -145,10 +145,6 @@ func (flags *installationPatchRequestOptions) GetPatchInstallationRequest() *mod
 		request.License = &flags.license
 	}
 
-	if flags.allowedIPRangesChanged {
-		request.AllowedIPRanges = &flags.allowedIPRanges
-	}
-
 	if flags.overrideIPRangesChanged {
 		request.OverrideIPRanges = &flags.overrideIPRanges
 	}
@@ -187,6 +183,16 @@ func (flags *installationUpdateFlags) GetPatchInstallationRequest() (*model.Patc
 		return nil, err
 	}
 
+	if flags.allowedIPRangesChanged {
+		allowedIPRanges := &model.AllowedIPRanges{}
+		allowedIPRanges.FromJSONString(flags.allowedIPRanges)
+		allowedIPRanges, err := allowedIPRanges.FromJSONString(flags.allowedIPRanges)
+		if err != nil {
+			return nil, err
+		}
+		request.AllowedIPRanges = allowedIPRanges
+	}
+
 	request.MattermostEnv = mattermostEnv
 	request.PriorityEnv = priorityEnv
 

diff --git a/internal/provisioner/cluster_installation_provisioner.go b/internal/provisioner/cluster_installation_provisioner.go
@@ -475,8 +475,8 @@ func (provisioner Provisioner) updateClusterInstallation(
 	mattermost.Spec.IngressName = ""
 	mattermost.Spec.IngressAnnotations = nil
 	annotations := mattermost.Spec.Ingress.Annotations
-	if installation.AllowedIPRanges != "" {
-		annotations = addInternalSourceRanges(annotations, installation.AllowedIPRanges, provisioner.params.InternalIPRanges)
+	if installation.AllowedIPRanges != nil && len(*installation.AllowedIPRanges) > 0 {
+		annotations = addInternalSourceRanges(annotations, installation.AllowedIPRanges.ToAnnotationString(), provisioner.params.InternalIPRanges)
 	}
 	mattermost.Spec.Ingress = makeIngressSpec(installationDNS, annotations)
 

diff --git a/internal/store/migrations.go b/internal/store/migrations.go
@@ -2174,6 +2174,21 @@ var migrations = []migration{
 			return errors.Wrap(err, "failed to add AllowedIPRanges column")
 		}
 
+		return nil
+	}},
+	{semver.MustParse("0.45.0"), semver.MustParse("0.46.0"), func(e execer) error {
+
+		_, err := e.Exec(`ALTER TABLE Installation DROP COLUMN AllowedIPRanges;`)
+		if err != nil {
+			return err
+		}
+
+		_, err = e.Exec(`ALTER TABLE Installation ADD COLUMN AllowedIPRanges JSON DEFAULT NULL;`)
+
+		if err != nil {
+			return err
+		}
+
 		return nil
 	}},
 }
diff --git a/model/installation.go b/model/installation.go
@@ -5,9 +5,13 @@
 package model
 
 import (
+	"database/sql/driver"
 	"encoding/json"
 	"fmt"
 	"io"
+	"strings"
+
+	"github.com/pkg/errors"
 )
 
 //go:generate provisioner-code-gen generate --out-file=installation_gen.go --boilerplate-file=../hack/boilerplate/boilerplate.generatego.txt --type=github.com/mattermost/mattermost-cloud/model.Installation --generator=get_id,get_state,is_deleted,as_resources
@@ -38,7 +42,7 @@ type Installation struct {
 	ExternalDatabaseConfig     *ExternalDatabaseConfig     `json:"ExternalDatabaseConfig,omitempty"`
 	Filestore                  string
 	License                    string
-	AllowedIPRanges            string
+	AllowedIPRanges            *AllowedIPRanges
 	MattermostEnv              EnvVarMap
 	PriorityEnv                EnvVarMap
 	Size                       string
@@ -81,6 +85,102 @@ type InstallationFilter struct {
 	Name            string
 }
 
+type AllowedIPRanges []AllowedIPRange
+
+type AllowedIPRange struct {
+	CIDRBlock   string
+	Description string
+	Enabled     bool
+	// TODO - necessary?
+	OwnerID string
+}
+
+func (a AllowedIPRanges) Value() (driver.Value, error) {
+	return json.Marshal(a)
+}
+
+func (a *AllowedIPRanges) Scan(src interface{}) error {
+	source, ok := src.([]byte)
+	if !ok {
+		return errors.New("Could not assert type of AllowedIPRanges")
+	}
+
+	var i AllowedIPRanges
+	err := json.Unmarshal(source, &i)
+	if err != nil {
+		return err
+	}
+	*a = i
+	return nil
+}
+
+func (a *AllowedIPRanges) FromJSONString(allowedIPRangesStr string) (*AllowedIPRanges, error) {
+	// Unmarshal the JSON into an AllowedIPRanges slice
+	var allowedIPRanges AllowedIPRanges
+	err := json.Unmarshal([]byte(allowedIPRangesStr), &allowedIPRanges)
+	if err != nil {
+		fmt.Printf("Error parsing JSON: %v\n", err)
+		return nil, err
+	}
+	return &allowedIPRanges, nil
+}
+
+func (a *AllowedIPRanges) ToString() string {
+	if a == nil {
+		return ""
+	}
+
+	b, err := json.Marshal(a)
+	if err != nil {
+		return ""
+	}
+
+	return string(b)
+}
+
+func (a *AllowedIPRanges) Contains(IP string) bool {
+	if a == nil {
+		return false
+	}
+
+	for _, allowedIPRange := range *a {
+		if allowedIPRange.CIDRBlock == IP {
+			return true
+		}
+	}
+
+	return false
+}
+
+func (a *AllowedIPRanges) ToAnnotationString() string {
+	if a == nil {
+		return ""
+	}
+
+	var IPs []string
+	for _, allowedIPRange := range *a {
+		IPs = append(IPs, allowedIPRange.CIDRBlock)
+	}
+
+	result := strings.Join(IPs, ",")
+	result = strings.TrimPrefix(result, ",")
+
+	return result
+}
+
+func (a *AllowedIPRanges) AreValid() bool {
+	if a == nil {
+		// Empty is valid
+		return true
+	}
+	for _, allowedIPRange := range *a {
+		if !IsIPRangeValid(allowedIPRange.CIDRBlock) {
+			return false
+		}
+	}
+	return true
+}
+
 // Clone returns a deep copy the installation.
 func (i *Installation) Clone() *Installation {
 	var clone Installation

diff --git a/model/installation_request.go b/model/installation_request.go
@@ -7,6 +7,7 @@ package model
 import (
 	"encoding/json"
 	"io"
+	"net"
 	"net/url"
 	"regexp"
 	"strings"
@@ -291,7 +292,7 @@ type GetInstallationsRequest struct {
 	State                       string
 	DNS                         string
 	Name                        string
-	AllowedIPRanges             string
+	AllowedIPRanges             *AllowedIPRanges
 	OverrideIPRanges            bool
 	IncludeGroupConfig          bool
 	IncludeGroupConfigOverrides bool
@@ -305,7 +306,7 @@ func (request *GetInstallationsRequest) ApplyToURL(u *url.URL) {
 	q.Add("state", request.State)
 	q.Add("dns_name", request.DNS)
 	q.Add("name", request.Name)
-	q.Add("allowed-ip-ranges", request.AllowedIPRanges)
+	q.Add("allowed-ip-ranges", request.AllowedIPRanges.ToString())
 	if !request.OverrideIPRanges {
 		q.Add("override-ip-ranges", "false")
 	}
@@ -327,7 +328,7 @@ type PatchInstallationRequest struct {
 	Version          *string
 	Size             *string
 	License          *string
-	AllowedIPRanges  *string
+	AllowedIPRanges  *AllowedIPRanges
 	OverrideIPRanges *bool
 	PriorityEnv      EnvVarMap
 	MattermostEnv    EnvVarMap
@@ -341,7 +342,7 @@ func (p *PatchInstallationRequest) Validate() error {
 	if p.Image != nil && len(*p.Image) == 0 {
 		return errors.New("provided image update value was blank")
 	}
-	if p.AllowedIPRanges != nil && len(*p.AllowedIPRanges) == 0 {
+	if !p.AllowedIPRanges.AreValid() {
 		return errors.New("provided ip ranges update value was blank")
 	}
 	if p.Size != nil {
@@ -381,14 +382,24 @@ func (p *PatchInstallationRequest) Apply(installation *Installation) bool {
 		installation.License = *p.License
 	}
 
-	if p.AllowedIPRanges != nil && *p.AllowedIPRanges != installation.AllowedIPRanges {
+	if p.AllowedIPRanges != nil && p.AllowedIPRanges != installation.AllowedIPRanges {
 		applied = true
 		if p.OverrideIPRanges != nil && *p.OverrideIPRanges {
-			installation.AllowedIPRanges = *p.AllowedIPRanges
+			allowedIPRanges, err := p.replaceIngressSourceRanges()
+			if err != nil {
+				return false
+			}
+			installation.AllowedIPRanges = allowedIPRanges
 		} else {
-			installation.AllowedIPRanges = p.addMissingIngressSourceRanges(installation)
+			allowedIPRanges, err := p.MergeNewIngressSourceRangesWithExisting(installation)
+			if err != nil {
+				return false
+			}
+			installation.AllowedIPRanges = allowedIPRanges
 		}
+
 	}
+
 	if p.MattermostEnv != nil {
 		if installation.MattermostEnv.ClearOrPatch(&p.MattermostEnv) {
 			applied = true
@@ -453,23 +464,48 @@ func (p *PatchInstallationDeletionRequest) Apply(installation *Installation) boo
 	return applied
 }
 
-func (p *PatchInstallationRequest) addMissingIngressSourceRanges(installation *Installation) string {
-	var ips []string
-	if p.AllowedIPRanges != nil {
-		ips = strings.Split(*p.AllowedIPRanges, ",")
+// MergeNewIngressSourceRangesWithExisting merges the AllowedIPRanges from the PatchInstallationRequest with the existing AllowedIPRanges from the Installation.
+// This is done so that individual fields of an AllowedIPRange (like the enabled field) can be adjusted without overriding the whole AllowedIPRanges slice.
+func (p *PatchInstallationRequest) MergeNewIngressSourceRangesWithExisting(installation *Installation) (*AllowedIPRanges, error) {
+	if p.AllowedIPRanges == nil {
+		return installation.AllowedIPRanges, nil
+	}
+
+	allowedRanges := *installation.AllowedIPRanges
+	patchAllowedRanges := *p.AllowedIPRanges
+
+	// Create a map to store the allowedRanges by CIDRBlock
+	allowedMap := make(map[string]AllowedIPRange)
+	for _, allowedRange := range allowedRanges {
+		allowedMap[allowedRange.CIDRBlock] = allowedRange
 	}
 
-	allowedIPRanges := strings.Split(installation.AllowedIPRanges, ",")
-	for _, ip := range ips {
-		if !contains(allowedIPRanges, ip) {
-			allowedIPRanges = append(allowedIPRanges, ip)
+	// Merge the patchAllowedRanges into the allowedMap
+	for _, patchRange := range patchAllowedRanges {
+		if !IsIPRangeValid(patchRange.CIDRBlock) {
+			return nil, errors.New("Invalid CIDR block provided")
 		}
+		allowedMap[patchRange.CIDRBlock] = patchRange
 	}
 
-	allowiplistRange := strings.Join(allowedIPRanges, ",")
-	allowiplistRange = strings.TrimPrefix(allowiplistRange, ",")
+	// Convert the map back into a slice
+	var mergedRanges AllowedIPRanges
+	for _, rangeValue := range allowedMap {
+		mergedRanges = append(mergedRanges, rangeValue)
+	}
+
+	return &mergedRanges, nil
+}
+
+// This function parses the InstallationRequests's AllowedIPRanges and returns an error if any of the ranges are invalid.
+func (p *PatchInstallationRequest) replaceIngressSourceRanges() (*AllowedIPRanges, error) {
+	for _, allowedIPRange := range *p.AllowedIPRanges {
+		if !IsIPRangeValid(allowedIPRange.CIDRBlock) {
+			return nil, errors.New("Invalid CIDR block provided")
+		}
+	}
 
-	return allowiplistRange
+	return p.AllowedIPRanges, nil
 }
 
 // NewPatchInstallationDeletionRequestFromReader will create a PatchInstallationDeletionRequest from an io.Reader with JSON data.
@@ -503,3 +539,30 @@ func NewAssignInstallationGroupRequestFromReader(reader io.Reader) (*AssignInsta
 
 	return &assignGroupRequest, nil
 }
+
+// IsIPRangeValid validates if an IP range is considered valid.
+func IsIPRangeValid(ipRange string) bool {
+	// Split IP range into parts
+	parts := strings.Split(ipRange, "/")
+	ipString := parts[0]
+
+	// Validate IP address
+	ip := net.ParseIP(ipString)
+	if ip == nil {
+		return false
+	}
+
+	// If no subnet mask provided, treat it as /32 (single IP)
+	if len(parts) == 1 {
+		return true
+	}
+
+	// Validate subnet mask
+	_, ipNet, err := net.ParseCIDR(ipRange)
+	if err != nil {
+		return false
+	}
+
+	// Check if the IP is within the specified subnet
+	return ipNet.Contains(ip)
+}