Say when --local reads a region never synced; open_buckets checks ACLs only

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

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) {

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())
 }
 

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

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.

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(&notes)
+	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()

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)
 }

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> <cloud:id> \"pol-1\" .\n<pol-1> <rdf:type> <cloud-owl:Policy> .\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.

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")
+	}
+}

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)))