From f53034174853f2f781df5d7336c4e0b42751749e Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Thu, 12 Sep 2024 18:14:09 -0300 Subject: [PATCH 1/8] [Fixed] Parse GPO ignoring when registry policy name is not Registry.pol Signed-off-by: Davi Henrique --- internal/ad/ad.go | 229 +++++++++++++++++++++++++++------------------- 1 file changed, 134 insertions(+), 95 deletions(-) diff --git a/internal/ad/ad.go b/internal/ad/ad.go index cf9bddfe2..aa338d7f0 100644 --- a/internal/ad/ad.go +++ b/internal/ad/ad.go @@ -491,116 +491,155 @@ func (ad *AD) parseGPOs(ctx context.Context, gpos []gpo, objectClass ObjectClass Rules: make(map[string][]entry.Entry), } r = append(r, gpoWithRules) - if err := func() error { - ad.downloadables[name].mu.RLock() - defer ad.downloadables[name].mu.RUnlock() - _ = ad.downloadables[name].testConcurrent - - log.Debugf(ctx, "Parsing GPO %q", name) - - // We need to consider the uppercase version of the name as well, - // which could occur in some of the default GPOs such as Default - // Domain Policy. - classes := []string{"User", "USER"} - if objectClass == ComputerObject { - classes = []string{"Machine", "MACHINE"} + if err = ad.parseGPO(ctx, name, url, keyFilterPrefix, objectClass, gpoWithRules); err != nil { + return r, err + } + } + + return r, nil +} + +func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, objectClass ObjectClass, gpoWithRules policies.GPO) error { + ad.downloadables[name].mu.RLock() + defer ad.downloadables[name].mu.RUnlock() + _ = ad.downloadables[name].testConcurrent + + log.Debugf(ctx, "Parsing GPO %q of class %q", name, objectClass) + + // We need to consider the uppercase version of the name as well, + // which could occur in some of the default GPOs such as Default + // Domain Policy. + classes := []string{"User", "USER"} + if objectClass == ComputerObject { + classes = []string{"Machine", "MACHINE"} + } + + var err error + var f *os.File + for _, class := range classes { + var e error + var files []os.DirEntry + + policyDir := filepath.Join(ad.sysvolCacheDir, "Policies", filepath.Base(url), class) + files, e = os.ReadDir(policyDir) + + if e != nil { + if errors.Is(e, fs.ErrNotExist) { + log.Debugf(ctx, "Not found policy directory %q", policyDir) + } else { + log.Warningf(ctx, "Failed to read policy directory %q: %s", policyDir, e) } + } else { + log.Debugf(ctx, "Success reading policy directory %q: found %d files", policyDir, len(files)) + } + + foundPolicy := false - var err error - var f *os.File - for _, class := range classes { - var e error - f, e = os.Open(filepath.Join(ad.sysvolCacheDir, "Policies", filepath.Base(url), class, "Registry.pol")) - - // We only care about the first error which is caused by opening - // the capitalized version of the class, instead of the - // uppercase version which is less common and more of an edge case. - if e != nil && err == nil { - err = e - } else if e == nil { - err = nil - break + //The filesystem might be case-sensitive, and if the registry policy is created as registry.pol + //it will not be found. + for _, file := range files { + if strings.EqualFold(file.Name(), "Registry.pol") { + policyPath := filepath.Join(policyDir, file.Name()) + log.Debugf(ctx, "Found registry policy file %q", policyPath) + + f, e = os.Open(policyPath) + + if e != nil { + log.Warningf(ctx, "Failed to open registry policy file %q", policyPath) + } else { + log.Debugf(ctx, "Success opening registry policy file %q", policyPath) + foundPolicy = true } - } - if errors.Is(err, fs.ErrNotExist) { - log.Debugf(ctx, "Policy %q doesn't have any policy for class %q %s", name, objectClass, err) - return nil - } else if err != nil { - return err + break } - defer decorate.LogFuncOnErrorContext(ctx, f.Close) + } - // Decode and apply policies in gpo order. First win - pols, err := registry.DecodePolicy(f) - if err != nil { - return errors.New(gotext.Get("%s: %v", f.Name(), err)) - } + if !foundPolicy && e == nil { + e = fs.ErrNotExist + } - // filter keys to be overridden - var currentKey string - var overrideEnabled bool - for _, pol := range pols { - // Rewrite the certificate autoenrollment key so we can easily - // use it in the policy manager - if pol.Key == certAutoEnrollKey { - pol.Key = fmt.Sprintf("%scertificate/autoenroll/all", keyFilterPrefix) - } + if e != nil && err == nil { + err = e + } else if e == nil { + err = nil + break + } + } - if strings.HasPrefix(pol.Key, policyServersPrefix) { - pol.Key = fmt.Sprintf("%scertificate/%s/all", keyFilterPrefix, pol.Key) - } + if errors.Is(err, fs.ErrNotExist) || f == nil { + log.Debugf(ctx, "Policy %q doesn't have any policy for class %q %s", name, objectClass, err) + return nil + } else if err != nil { + return err + } + defer decorate.LogFuncOnErrorContext(ctx, f.Close) - // Only consider supported policies for this distro - if !strings.HasPrefix(pol.Key, keyFilterPrefix) { - continue - } - if pol.Err != nil { - return errors.New(gotext.Get("%s: %v", f.Name(), pol.Err)) - } - pol.Key = strings.TrimPrefix(pol.Key, keyFilterPrefix) - - // Some keys can be overridden - releaseID := filepath.Base(pol.Key) - keyType := strings.Split(pol.Key, "/")[0] - pol.Key = filepath.Dir(strings.TrimPrefix(pol.Key, keyType+"/")) - - if releaseID == "all" { - currentKey = pol.Key - overrideEnabled = false - gpoWithRules.Rules[keyType] = append(gpoWithRules.Rules[keyType], pol) - continue - } + // Decode and apply policies in gpo order. First win + pols, err := registry.DecodePolicy(f) + if err != nil { + return errors.New(gotext.Get("%s: %v", f.Name(), err)) + } - // This is not an "all" key and the key name don’t match - // This shouldn’t happen with our admx, but just to stay safe… - if currentKey != pol.Key { - continue - } + // filter keys to be overridden + var currentKey string + var overrideEnabled bool + for _, pol := range pols { + // Rewrite the certificate autoenrollment key so we can easily + // use it in the policy manager + if pol.Key == certAutoEnrollKey { + pol.Key = fmt.Sprintf("%scertificate/autoenroll/all", keyFilterPrefix) + } - if strings.HasPrefix(releaseID, "Override"+ad.versionID) && pol.Value == "true" { - overrideEnabled = true - continue - } - // Check we have a matching override - if !overrideEnabled || releaseID != ad.versionID { - continue - } + if strings.HasPrefix(pol.Key, policyServersPrefix) { + pol.Key = fmt.Sprintf("%scertificate/%s/all", keyFilterPrefix, pol.Key) + } - // Matching enabled override - // Replace value with the override content - iLast := len(gpoWithRules.Rules[keyType]) - 1 - p := gpoWithRules.Rules[keyType][iLast] - p.Value = pol.Value - gpoWithRules.Rules[keyType][iLast] = p - } - return nil - }(); err != nil { - return r, err + // Only consider supported policies for this distro + if !strings.HasPrefix(pol.Key, keyFilterPrefix) { + continue + } + if pol.Err != nil { + return errors.New(gotext.Get("%s: %v", f.Name(), pol.Err)) } + pol.Key = strings.TrimPrefix(pol.Key, keyFilterPrefix) + + // Some keys can be overridden + releaseID := filepath.Base(pol.Key) + keyType := strings.Split(pol.Key, "/")[0] + pol.Key = filepath.Dir(strings.TrimPrefix(pol.Key, keyType+"/")) + + if releaseID == "all" { + currentKey = pol.Key + overrideEnabled = false + gpoWithRules.Rules[keyType] = append(gpoWithRules.Rules[keyType], pol) + continue + } + + // This is not an "all" key and the key name don’t match + // This shouldn’t happen with our admx, but just to stay safe… + if currentKey != pol.Key { + continue + } + + if strings.HasPrefix(releaseID, "Override"+ad.versionID) && pol.Value == "true" { + overrideEnabled = true + continue + } + // Check we have a matching override + if !overrideEnabled || releaseID != ad.versionID { + continue + } + + // Matching enabled override + // Replace value with the override content + iLast := len(gpoWithRules.Rules[keyType]) - 1 + p := gpoWithRules.Rules[keyType][iLast] + p.Value = pol.Value + gpoWithRules.Rules[keyType][iLast] = p } - return r, nil + return nil } // GetInfo returns all information from the selected backend: static and dynamic part. From 769707c0b2310cc24a3a203684e903448f78c57d Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Fri, 13 Sep 2024 11:13:00 -0300 Subject: [PATCH 2/8] [Changed] AD parse GPO find policy comment Co-authored-by: Didier Roche-Tolomelli --- internal/ad/ad.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ad/ad.go b/internal/ad/ad.go index aa338d7f0..d5f9fd668 100644 --- a/internal/ad/ad.go +++ b/internal/ad/ad.go @@ -535,7 +535,7 @@ func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, o foundPolicy := false - //The filesystem might be case-sensitive, and if the registry policy is created as registry.pol + // Registry.pol can have different cases, ensure we can find it whatever its case is //it will not be found. for _, file := range files { if strings.EqualFold(file.Name(), "Registry.pol") { From 0694601ba2cfa5d89832b65061cc45087b99e29f Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Fri, 13 Sep 2024 10:38:38 -0300 Subject: [PATCH 3/8] [Changed] AD parse GPO to fail fast on class directory list Signed-off-by: Davi Henrique --- internal/ad/ad.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/internal/ad/ad.go b/internal/ad/ad.go index d5f9fd668..b446ccb46 100644 --- a/internal/ad/ad.go +++ b/internal/ad/ad.go @@ -523,14 +523,11 @@ func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, o policyDir := filepath.Join(ad.sysvolCacheDir, "Policies", filepath.Base(url), class) files, e = os.ReadDir(policyDir) - if e != nil { - if errors.Is(e, fs.ErrNotExist) { - log.Debugf(ctx, "Not found policy directory %q", policyDir) - } else { - log.Warningf(ctx, "Failed to read policy directory %q: %s", policyDir, e) - } - } else { - log.Debugf(ctx, "Success reading policy directory %q: found %d files", policyDir, len(files)) + if errors.Is(e, fs.ErrNotExist) { + log.Debugf(ctx, "Policy directory %q not found", policyDir) + continue + } else if e != nil { + return e } foundPolicy := false From 16ca52e69c6515943efe956d027a476f263f9eb7 Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Fri, 13 Sep 2024 10:41:24 -0300 Subject: [PATCH 4/8] [Fixed] AD parse GPO found policy not considering all classes Signed-off-by: Davi Henrique --- internal/ad/ad.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/ad/ad.go b/internal/ad/ad.go index b446ccb46..e19626435 100644 --- a/internal/ad/ad.go +++ b/internal/ad/ad.go @@ -530,10 +530,7 @@ func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, o return e } - foundPolicy := false - // Registry.pol can have different cases, ensure we can find it whatever its case is - //it will not be found. for _, file := range files { if strings.EqualFold(file.Name(), "Registry.pol") { policyPath := filepath.Join(policyDir, file.Name()) From 2d9b1a3b239794c9e51c8ab09eb80ea248fb23e0 Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Fri, 13 Sep 2024 10:46:03 -0300 Subject: [PATCH 5/8] [Removed] AD parse GPO if nested scope Signed-off-by: Davi Henrique --- internal/ad/ad.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/internal/ad/ad.go b/internal/ad/ad.go index e19626435..1882120a0 100644 --- a/internal/ad/ad.go +++ b/internal/ad/ad.go @@ -532,21 +532,23 @@ func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, o // Registry.pol can have different cases, ensure we can find it whatever its case is for _, file := range files { - if strings.EqualFold(file.Name(), "Registry.pol") { - policyPath := filepath.Join(policyDir, file.Name()) - log.Debugf(ctx, "Found registry policy file %q", policyPath) + if !strings.EqualFold(file.Name(), "Registry.pol") { + continue + } - f, e = os.Open(policyPath) + policyPath := filepath.Join(policyDir, file.Name()) + log.Debugf(ctx, "Found registry policy file %q", policyPath) - if e != nil { - log.Warningf(ctx, "Failed to open registry policy file %q", policyPath) - } else { - log.Debugf(ctx, "Success opening registry policy file %q", policyPath) - foundPolicy = true - } + f, e = os.Open(policyPath) - break + if e != nil { + log.Warningf(ctx, "Failed to open registry policy file %q", policyPath) + } else { + log.Debugf(ctx, "Success opening registry policy file %q", policyPath) + foundPolicy = true } + + break } if !foundPolicy && e == nil { From a648622a1f868c278c2f27aa96a9ec6787e45f8a Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Fri, 13 Sep 2024 11:10:51 -0300 Subject: [PATCH 6/8] [Simplified] AD parse GPO find policy Signed-off-by: Davi Henrique --- internal/ad/ad.go | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/internal/ad/ad.go b/internal/ad/ad.go index 1882120a0..54a9ac733 100644 --- a/internal/ad/ad.go +++ b/internal/ad/ad.go @@ -514,20 +514,20 @@ func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, o classes = []string{"Machine", "MACHINE"} } - var err error var f *os.File +loop: for _, class := range classes { - var e error + var err error var files []os.DirEntry policyDir := filepath.Join(ad.sysvolCacheDir, "Policies", filepath.Base(url), class) - files, e = os.ReadDir(policyDir) + files, err = os.ReadDir(policyDir) - if errors.Is(e, fs.ErrNotExist) { + if errors.Is(err, fs.ErrNotExist) { log.Debugf(ctx, "Policy directory %q not found", policyDir) continue - } else if e != nil { - return e + } else if err != nil { + return err } // Registry.pol can have different cases, ensure we can find it whatever its case is @@ -539,36 +539,21 @@ func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, o policyPath := filepath.Join(policyDir, file.Name()) log.Debugf(ctx, "Found registry policy file %q", policyPath) - f, e = os.Open(policyPath) + f, err = os.Open(policyPath) - if e != nil { - log.Warningf(ctx, "Failed to open registry policy file %q", policyPath) - } else { - log.Debugf(ctx, "Success opening registry policy file %q", policyPath) - foundPolicy = true + if err != nil { + return err } - break - } - - if !foundPolicy && e == nil { - e = fs.ErrNotExist - } - - if e != nil && err == nil { - err = e - } else if e == nil { - err = nil - break + break loop } } - if errors.Is(err, fs.ErrNotExist) || f == nil { - log.Debugf(ctx, "Policy %q doesn't have any policy for class %q %s", name, objectClass, err) + if f == nil { + log.Debugf(ctx, "Policy %q doesn't have any policy for class %q", name, objectClass) return nil - } else if err != nil { - return err } + defer decorate.LogFuncOnErrorContext(ctx, f.Close) // Decode and apply policies in gpo order. First win From f8a9e9551e05eb6f6558025c2a14cbe7aea33833 Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Mon, 16 Sep 2024 12:04:31 -0300 Subject: [PATCH 7/8] [Renamed] AD parse GPO label loop Signed-off-by: Davi Henrique --- internal/ad/ad.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/ad/ad.go b/internal/ad/ad.go index 54a9ac733..2e3308b92 100644 --- a/internal/ad/ad.go +++ b/internal/ad/ad.go @@ -515,14 +515,13 @@ func (ad *AD) parseGPO(ctx context.Context, name, url, keyFilterPrefix string, o } var f *os.File -loop: +classLoop: for _, class := range classes { var err error var files []os.DirEntry policyDir := filepath.Join(ad.sysvolCacheDir, "Policies", filepath.Base(url), class) files, err = os.ReadDir(policyDir) - if errors.Is(err, fs.ErrNotExist) { log.Debugf(ctx, "Policy directory %q not found", policyDir) continue @@ -540,12 +539,11 @@ loop: log.Debugf(ctx, "Found registry policy file %q", policyPath) f, err = os.Open(policyPath) - if err != nil { return err } - break loop + break classLoop } } From 7703b2f5c4a25df44920c3ee637280e26ee81108 Mon Sep 17 00:00:00 2001 From: Davi Henrique Date: Mon, 16 Sep 2024 12:20:21 -0300 Subject: [PATCH 8/8] [Added] Tests for registry.pol spelling cases Signed-off-by: Davi Henrique --- internal/ad/ad_test.go | 22 +++++++++++++++++- .../Policies/lowercase-registry/GPT.INI | 3 +++ .../lowercase-registry/Machine/registry.pol | Bin 0 -> 350 bytes .../lowercase-registry/User/registry.pol | Bin 0 -> 350 bytes .../Policies/mixedcase-registry/GPT.INI | 3 +++ .../mixedcase-registry/Machine/reGIStry.pol | Bin 0 -> 350 bytes .../mixedcase-registry/User/reGIStry.pol | Bin 0 -> 350 bytes 7 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/GPT.INI create mode 100644 internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/Machine/registry.pol create mode 100644 internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/User/registry.pol create mode 100644 internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/mixedcase-registry/GPT.INI create mode 100644 internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/mixedcase-registry/Machine/reGIStry.pol create mode 100644 internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/mixedcase-registry/User/reGIStry.pol diff --git a/internal/ad/ad_test.go b/internal/ad/ad_test.go index 14ee83e84..d6ebd5038 100644 --- a/internal/ad/ad_test.go +++ b/internal/ad/ad_test.go @@ -424,7 +424,7 @@ func TestGetPolicies(t *testing.T) { }}, }, - // Policy class directory spelling cases + // Policy class directory and Registry.pol spelling cases "Policy user directory is uppercase": { gpoListArgs: []string{"gpoonly.com", "bob:uppercase-class"}, want: policies.Policies{GPOs: []policies.GPO{standardUserGPO("uppercase-class")}}, @@ -450,6 +450,26 @@ func TestGetPolicies(t *testing.T) { {ID: "lowercase-class", Name: "lowercase-class-name", Rules: map[string][]entry.Entry{}}}, }, }, + "User policy Registry.pol is lower case": { + gpoListArgs: []string{"gpoonly.com", "bob:lowercase-registry"}, + want: policies.Policies{GPOs: []policies.GPO{standardUserGPO("lowercase-registry")}}, + }, + "Computer policy Registry.pol is lower case": { + objectName: hostname, + objectClass: ad.ComputerObject, + gpoListArgs: []string{"gpoonly.com", hostname + ":lowercase-registry"}, + want: policies.Policies{GPOs: []policies.GPO{standardComputerGPO("lowercase-registry")}}, + }, + "User policy Registry.pol is mixed case": { + gpoListArgs: []string{"gpoonly.com", "bob:mixedcase-registry"}, + want: policies.Policies{GPOs: []policies.GPO{standardUserGPO("mixedcase-registry")}}, + }, + "Computer policy Registry.pol is mixed case": { + objectName: hostname, + objectClass: ad.ComputerObject, + gpoListArgs: []string{"gpoonly.com", hostname + ":mixedcase-registry"}, + want: policies.Policies{GPOs: []policies.GPO{standardComputerGPO("mixedcase-registry")}}, + }, // Error cases "Machine doesn’t match": { diff --git a/internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/GPT.INI b/internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/GPT.INI new file mode 100644 index 000000000..c5498a917 --- /dev/null +++ b/internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/GPT.INI @@ -0,0 +1,3 @@ +[General] +Version=1000 +displayName=New Group Policy Object diff --git a/internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/Machine/registry.pol b/internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/lowercase-registry/Machine/registry.pol new file mode 100644 index 0000000000000000000000000000000000000000..3b1e3b69db3764697f840f687c5665cd227c382b GIT binary patch literal 350 zcmbu2OA3H63`C!I@HSlux);TjcA>V4APRo)`c8WTk&uwj%S>7MIRRQ~b|x+!dJa4t zr8X-IqtbI~(=c$eiMlm8U3N{A`b-)|pQ|EtLDP`NdW!GGxv1V4APRo)`c8WTk&uwj%S>7MIRRQ~b|x+!dJa4t zr8X-IqtbI~(=c$eiMlm8U3N{A`b-)|pQ|EtLDP`NdW!GGxv1