diff --git a/CHANGELOG.md b/CHANGELOG.md index f9ba9cd7..7c7bb4ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## v0.6.1 + +### Fixed + +- `list --local` says when the service it reads was never synced in that region, also + when another region of the profile was. A sync of us-west-2 left the global files + behind, and `list instances --local -r eu-west-1` answered an empty list. +- With `--format json`, the "not synced" hint is a plain line on stderr, like the + inspectors' notes, instead of an `[info]` log line. +- `inspect -i open_buckets` says that it reads bucket ACLs only, not bucket policies + or Block Public Access, so an empty answer no longer reads as "no public buckets". + ## v0.6.0 ### Added diff --git a/commands/list.go b/commands/list.go index c185f2b0..7ab44472 100644 --- a/commands/list.go +++ b/commands/list.go @@ -113,8 +113,8 @@ var listSpecificResourceCmd = func(resType string) *cobra.Command { var g cloud.GraphAPI if localGlobalFlag { - warnIfNothingSynced() if srvName, ok := awsservices.ServicePerResourceType[resType]; ok { + warnIfNotSynced(srvName) var err error g, err = sync.LoadLocalGraphForService(srvName, config.GetAWSProfile(), config.GetAWSRegion()) exitOn(err) @@ -142,6 +142,7 @@ var listAllResourceInServiceCmd = func(srvName string) *cobra.Command { Hidden: true, Run: func(cmd *cobra.Command, args []string) { + warnIfNotSynced(srvName) g, err := sync.LoadLocalGraphForService(srvName, config.GetAWSProfile(), config.GetAWSRegion()) exitOn(err) displayer, err := console.BuildOptions( @@ -161,16 +162,32 @@ var listAllResourceInServiceCmd = func(srvName string) *cobra.Command { } } -// warnIfNothingSynced says so when --local is asked to read a local copy that does -// not exist yet, instead of letting the command answer "No results found." — which -// says something about the account, in a situation where the account was never read. -func warnIfNothingSynced() { +// warnIfNotSynced says so when --local is asked to read a local copy that does not +// exist yet, instead of letting the command answer "No results found." — which says +// something about the account, in a situation where the account was never read. The +// copy is per service and region: a sync of another region leaves the global files +// behind, which are no answer for this region's instances. In JSON the hint is a note, +// a plain stderr line like the inspectors' notes, so that a caller who keeps the logs +// out of the result still gets it. +func warnIfNotSynced(service string) { profile, region := config.GetAWSProfile(), config.GetAWSRegion() - if !sync.NothingSyncedFor(profile, region) { + var msg string + switch { + case sync.NothingSyncedFor(profile, region): + msg = fmt.Sprintf("nothing has been synced for profile '%s' in region '%s' yet, so --local has "+ + "nothing to read. Run `awless-ro sync`, or drop --local to ask AWS directly.", profile, region) + case !sync.ServiceSyncedFor(service, profile, region): + msg = fmt.Sprintf("'%s' has not been synced for profile '%s' in region '%s' yet, so --local has "+ + "nothing to read for it. Run `awless-ro sync --%s`, or drop --local to ask AWS directly.", + service, profile, region, service) + default: return } - logger.Infof("nothing has been synced for profile '%s' in region '%s' yet, so --local has "+ - "nothing to read. Run `awless-ro sync`, or drop --local to ask AWS directly.", profile, region) + if jsonOutput() { + fmt.Fprintln(os.Stderr, msg) + return + } + logger.Info(msg) } func printResources(g cloud.GraphAPI, resType string) { diff --git a/commands/ssh.go b/commands/ssh.go index 5be7e47d..e0b6a8a6 100644 --- a/commands/ssh.go +++ b/commands/ssh.go @@ -216,7 +216,7 @@ func connectionGraph(local bool, infra infraFetcher, loadLocal func() (cloud.Gra // localInfraGraph reads the synced infra file, which holds the instances, their // security groups and the account's images. func localInfraGraph() (cloud.GraphAPI, error) { - warnIfNothingSynced() + warnIfNotSynced("infra") return sync.LoadLocalGraphForService("infra", config.GetAWSProfile(), config.GetAWSRegion()) } diff --git a/config/version.go b/config/version.go index e7140cb5..13e7f2aa 100644 --- a/config/version.go +++ b/config/version.go @@ -35,7 +35,7 @@ const AWLESS_ASCII_LOGO = ` // (no write operations), so continuing upstream's v0.1.11 numbering would imply // a compatibility that does not exist. var ( - Version = "v0.6.0" + Version = "v0.6.1" BuildFor string buildSha, buildDate, buildArch, buildOS string diff --git a/docs/release-v0.6.1.md b/docs/release-v0.6.1.md new file mode 100644 index 00000000..f33150ae --- /dev/null +++ b/docs/release-v0.6.1.md @@ -0,0 +1,27 @@ +# awless-ro v0.6.1 + +Fixes for answers that looked complete and were not, found reading the synced copy +through `--format json`. + +This is the OpenSystemsLab build of awless-ro. It is what the awless MCP server runs. + +### Fixed + +- `list --local` says when the service it reads was never synced in that region, also + when another region of the profile was. A sync of us-west-2 left the global files + behind, and `list instances --local -r eu-west-1` answered an empty list. +- With `--format json`, that hint is a plain line on stderr, like the inspectors' + notes, instead of an `[info]` log line. +- `inspect -i open_buckets` says that it reads bucket ACLs only, not bucket policies + or Block Public Access, so an empty answer no longer reads as "no public buckets". + +### Install + +Download the archive for your platform below and check it: + +```sh +shasum -a 256 -c SHA256SUMS --ignore-missing +``` + +Not on Homebrew: `brew install theazz/tap/awless-ro` installs the upstream release, +which does not have JSON output yet. diff --git a/inspect/inspectors/json_test.go b/inspect/inspectors/json_test.go index a1808fa2..536e752e 100644 --- a/inspect/inspectors/json_test.go +++ b/inspect/inspectors/json_test.go @@ -87,6 +87,26 @@ func TestOpenBucketsRowsGolden(t *testing.T) { } } +// Only ACL grants are read. An empty answer says nothing about bucket policies or +// Block Public Access, which is how buckets are opened nowadays, so the notes say so +// whatever the rows hold. +func TestOpenBucketsNotesSayWhatWasChecked(t *testing.T) { + open := &OpenBuckets{} + if err := open.Inspect(graph.NewGraph()); err != nil { + t.Fatal(err) + } + var notes bytes.Buffer + open.Rows(¬es) + if !strings.Contains(notes.String(), "ACL") || !strings.Contains(notes.String(), "Bucket policies") { + t.Errorf("notes should say only ACLs were checked, got %q", notes.String()) + } + var text bytes.Buffer + open.Print(&text) + if !strings.Contains(text.String(), "Bucket policies") { + t.Errorf("text output should say only ACLs were checked, got %q", text.String()) + } +} + func TestPortScannerRowsGolden(t *testing.T) { _, all, _ := net.ParseCIDR("0.0.0.0/0") g := graph.NewGraph() diff --git a/inspect/inspectors/open_buckets.go b/inspect/inspectors/open_buckets.go index e53b701f..5581eb77 100644 --- a/inspect/inspectors/open_buckets.go +++ b/inspect/inspectors/open_buckets.go @@ -81,7 +81,16 @@ type OpenBucket struct { Grantee string `json:"grantee"` } -func (a *OpenBuckets) Rows(io.Writer) interface{} { +// aclOnly is what the inspector cannot see. It reads the grants in bucket ACLs, and +// buckets are made public today through bucket policies, which it does not read, so an +// empty answer must not pass for "no bucket is public". +const aclOnly = "Only bucket ACL grants were checked. Bucket policies and S3 Block Public Access settings " + + "are not read, so a bucket made public by its policy is not listed here." + +func (a *OpenBuckets) Rows(notes io.Writer) interface{} { + if notes != nil { + fmt.Fprintln(notes, aclOnly) + } rows := []OpenBucket{} for _, b := range a.openToAny { rows = append(rows, OpenBucket{Bucket: b, Grantee: "AllUsers"}) @@ -104,4 +113,5 @@ func (a *OpenBuckets) Print(w io.Writer) { if len(a.openToAnyAuth) == 0 && len(a.openToAny) == 0 { fmt.Fprintln(w, "none found") } + fmt.Fprintln(w, aclOnly) } diff --git a/main_test.go b/main_test.go index 23a43705..b600ad08 100644 --- a/main_test.go +++ b/main_test.go @@ -496,6 +496,49 @@ func TestLocalListNothingSynced(t *testing.T) { } } +// A region that was never synced, in a profile that was: the global access file from a +// sync of another region must not pass for this region's infra. The listing used to +// answer an empty list and nothing else. +func TestLocalListServiceNotSyncedInRegion(t *testing.T) { + bin := build(t) + home := graphHome(t, map[string]string{ + "global/access": " \"pol-1\" .\n .\n", + }) + + stdout, stderr, exit := runLocal(t, bin, home, "list", "instances", "--local") + if exit != 0 { + t.Errorf("exit = %d, want 0", exit) + } + if !strings.Contains(stdout+stderr, "'infra' has not been synced") { + t.Errorf("expected a hint that infra was not synced in this region, got stdout:\n%s\nstderr:\n%s", stdout, stderr) + } + + // In JSON the hint is a note, a plain stderr line like the inspectors' notes, not a + // log line that a caller filtering logs out would drop. + stdout, stderr, exit = runLocal(t, bin, home, "list", "instances", "--local", "--format", "json") + if exit != 0 { + t.Errorf("json: exit = %d, want 0", exit) + } + if strings.TrimSpace(stdout) != "[]" { + t.Errorf("json: stdout = %q, want []", stdout) + } + var note string + for _, line := range strings.Split(stderr, "\n") { + if strings.Contains(line, "'infra' has not been synced") { + note = line + } + } + if note == "" || strings.HasPrefix(note, "[") { + t.Errorf("json: want a plain note on stderr, got:\n%s", stderr) + } + + // The access file is there, so policies list without the hint. + _, stderr, _ = runLocal(t, bin, home, "list", "policies", "--local", "--format", "json") + if strings.Contains(stderr, "has not been synced") { + t.Errorf("policies were synced, yet: %s", stderr) + } +} + // Part 2 end to end: a 9 MiB document literal — far past the old scanner ceiling — loads // and lists. `list` does not render the document value itself, so this is the end-to-end // proof that the decoder now reads an arbitrarily large single line. diff --git a/sync/nothingsynced_test.go b/sync/nothingsynced_test.go index 59777cca..a758966e 100644 --- a/sync/nothingsynced_test.go +++ b/sync/nothingsynced_test.go @@ -63,3 +63,32 @@ func writeGraph(t *testing.T, home, profile, regionDir, service string) { t.Fatal(err) } } + +// A profile synced in one region has global files everywhere, so NothingSyncedFor is no +// answer to "was this region's infra synced"; ServiceSyncedFor is. +func TestServiceSyncedFor(t *testing.T) { + home := t.TempDir() + t.Setenv("__AWLESS_HOME", home) + const profile = "admin" + + writeGraph(t, home, profile, "global", "access") + writeGraph(t, home, profile, "us-west-2", "infra") + + for _, c := range []struct { + service, region string + want bool + }{ + {"infra", "us-west-2", true}, + {"infra", "eu-west-1", false}, + {"storage", "us-west-2", false}, + {"access", "eu-west-1", true}, // global, whatever the region + {"dns", "us-west-2", false}, + } { + if got := ServiceSyncedFor(c.service, profile, c.region); got != c.want { + t.Errorf("ServiceSyncedFor(%s, %s) = %v, want %v", c.service, c.region, got, c.want) + } + } + if NothingSyncedFor(profile, "eu-west-1") { + t.Error("precondition: the global file counts as something synced") + } +} diff --git a/sync/sync.go b/sync/sync.go index 6c54ddd7..a560eb39 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -202,12 +202,7 @@ func (e *ServiceError) Unwrap() error { return e.Err } // closed instead of presenting unread data as "No results found.". The returned // graph is always non-nil so a caller that ignores the error cannot nil-deref. func LoadLocalGraphForService(serviceName, profile, region string) (cloud.GraphAPI, error) { - regionDir := region - if serviceName == "access" || serviceName == "dns" || serviceName == "cdn" { - regionDir = "global" - } - path := filepath.Join(repo.BaseDir(), profile, regionDir, fmt.Sprintf("%s%s", serviceName, fileExt)) - g, err := graph.NewGraphFromFiles(path) + g, err := graph.NewGraphFromFiles(localGraphPath(serviceName, profile, region)) if err != nil { if errors.Is(err, fs.ErrNotExist) { return graph.NewGraph(), nil @@ -217,6 +212,22 @@ func LoadLocalGraphForService(serviceName, profile, region string) (cloud.GraphA return g, nil } +// ServiceSyncedFor reports whether the service's file for this profile and region has +// been synced. A profile synced in one region has its global files (access, dns, cdn) +// for every region, so "something was synced" does not mean this region's infra was. +func ServiceSyncedFor(serviceName, profile, region string) bool { + _, err := os.Stat(localGraphPath(serviceName, profile, region)) + return err == nil +} + +func localGraphPath(serviceName, profile, region string) string { + regionDir := region + if serviceName == "access" || serviceName == "dns" || serviceName == "cdn" { + regionDir = "global" + } + return filepath.Join(repo.BaseDir(), profile, regionDir, fmt.Sprintf("%s%s", serviceName, fileExt)) +} + func LoadLocalGraphs(profile, region string) (cloud.GraphAPI, error) { var files []string globalFiles, _ := filepath.Glob(filepath.Join(repo.BaseDir(), profile, "global", fmt.Sprintf("*%s", fileExt)))