diff --git a/README.md b/README.md index ef55fe7..e92b0ff 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,14 @@ For the moment, it is solely limited to authenticated Github organizations with - `read:org` for the organization to be queried. Note - you _might_ need to be an administrator of the GH Org to work correctly - `read:members` to be able to read teams +### IP allow-list data + +GitHub organization IP allow-list collection is disabled by default. Enable it only when the plugin is configured with a Classic Personal Access Token with organization administrator permissions. Classic PATs are broad credentials, so prefer leaving `collect_ip_allow_list` set to `false` unless IP allow-list policies are required. + +Fine-grained Personal Access Tokens and GitHub App installation tokens may be able to read other organization settings, but GitHub does not currently allow them to query `organization.ipAllowListEntries` through GraphQL. + +If IP allow-list data cannot be fetched, the plugin continues with partial data and policies that depend on `ip_allow_list` should report a skip reason instead of evaluating incomplete evidence. + ## Building Once you are ready to serve the plugin, you need to build the binaries which can be used by the agent. @@ -37,6 +45,7 @@ In the example above, setting an empty token, and an environment variable `CCF_P ```shell export CCF_PLUGINS_GITHUB_CONFIG_TOKEN="github_pat_1234..." +export CCF_PLUGINS_GITHUB_CONFIG_COLLECT_IP_ALLOW_LIST="false" ``` ```yaml @@ -46,6 +55,7 @@ plugins: config: token: "" # Will be read from the CCF_PLUGINS_GITHUB_CONFIG_TOKEN environment variable organization: test-org # The name of the organization + collect_ip_allow_list: false # Set to true only when using a Classic PAT and IP allow-list evidence is required ``` ## Releasing diff --git a/go.mod b/go.mod index 79fc3be..8697284 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/hashicorp/go-hclog v1.6.3 github.com/hashicorp/go-plugin v1.7.0 github.com/mitchellh/mapstructure v1.5.0 + github.com/open-policy-agent/opa v1.14.1 ) require ( @@ -36,7 +37,6 @@ require ( github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.20 // indirect github.com/oklog/run v1.2.0 // indirect - github.com/open-policy-agent/opa v1.14.1 // indirect github.com/rcrowley/go-metrics v0.0.0-20250401214520-65e299d6c5c9 // indirect github.com/segmentio/asm v1.2.1 // indirect github.com/sirupsen/logrus v1.9.4 // indirect diff --git a/internal/data.go b/internal/data.go index dcb9149..9c84d97 100644 --- a/internal/data.go +++ b/internal/data.go @@ -2,6 +2,7 @@ package internal import ( "context" + "errors" "fmt" "net/http" @@ -29,7 +30,7 @@ type GithubData struct { Teams []*github.Team `json:"teams"` Members []*github.User `json:"members"` SSO *OrgSSO `json:"sso"` - IPAllowList []IPAllowListEntry `json:"ip_allow_list"` + IPAllowList *[]IPAllowListEntry `json:"ip_allow_list"` } type DataFetcher struct { @@ -44,7 +45,7 @@ func NewDataFetcher(logger hclog.Logger, client *github.Client) *DataFetcher { } } -func (df DataFetcher) FetchData(ctx context.Context, organization string) (*GithubData, []*proto.Step, error) { +func (df DataFetcher) FetchData(ctx context.Context, organization string, collectIPAllowList bool) (*GithubData, []*proto.Step, error) { steps := make([]*proto.Step, 0) steps = append(steps, &proto.Step{ @@ -76,11 +77,13 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith Remarks: policy_manager.Pointer("More information: https://docs.github.com/en/enterprise-cloud@latest/organizations/managing-saml-single-sign-on-for-your-organization/about-identity-and-access-management-with-saml-single-sign-on"), }) - steps = append(steps, &proto.Step{ - Title: "Get IP Allow-List", - Description: "Fetches the IP allow-list entries for the organization via the GitHub GraphQL API", - Remarks: policy_manager.Pointer("More information: https://docs.github.com/en/graphql/reference/objects#ipallowlistentry"), - }) + if collectIPAllowList { + steps = append(steps, &proto.Step{ + Title: "Get IP Allow-List", + Description: "Fetches the IP allow-list entries for the organization via the GitHub GraphQL API", + Remarks: policy_manager.Pointer("More information: https://docs.github.com/en/graphql/reference/objects#ipallowlistentry"), + }) + } org, _, err := df.client.Organizations.Get(ctx, organization) if err != nil { @@ -88,14 +91,16 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith return nil, nil, err } - var allTeams []*github.Team + var accumulatedErrors error + allTeams := make([]*github.Team, 0) paginationOpt := &github.ListOptions{PerPage: 100} for { teams, resp, err := df.client.Teams.ListTeams(ctx, organization, paginationOpt) if err != nil { - df.logger.Error("Error getting teams information", "org", organization, "error", err) - return nil, nil, err + df.logger.Warn("Skipping teams collection after GitHub API error", "org", organization, "error", err) + accumulatedErrors = errors.Join(accumulatedErrors, fmt.Errorf("failed to fetch teams: %w", err)) + break } allTeams = append(allTeams, teams...) @@ -105,7 +110,7 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith paginationOpt.Page = resp.NextPage } - var allAdminMembers []*github.User + allAdminMembers := make([]*github.User, 0) memberOpt := &github.ListMembersOptions{ Role: "admin", ListOptions: github.ListOptions{PerPage: 100}, @@ -114,8 +119,9 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith for { members, resp, err := df.client.Organizations.ListMembers(ctx, organization, memberOpt) if err != nil { - df.logger.Error("Error getting admin members", "org", organization, "error", err) - return nil, nil, err + df.logger.Warn("Skipping admin member collection after GitHub API error", "org", organization, "error", err) + accumulatedErrors = errors.Join(accumulatedErrors, fmt.Errorf("failed to fetch admin members: %w", err)) + break } allAdminMembers = append(allAdminMembers, members...) @@ -127,14 +133,19 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith ssoData, err := df.fetchSSO(ctx, organization) if err != nil { - df.logger.Error("Error getting SSO configuration", "org", organization, "error", err) - return nil, nil, err + df.logger.Warn("Skipping SSO collection after GitHub API error", "org", organization, "error", err) + accumulatedErrors = errors.Join(accumulatedErrors, fmt.Errorf("failed to fetch SSO configuration: %w", err)) + ssoData = nil } - ipAllowList, err := df.fetchIPAllowList(ctx, organization) - if err != nil { - df.logger.Error("Error getting IP allow-list", "org", organization, "error", err) - return nil, nil, err + var ipAllowList []IPAllowListEntry + if collectIPAllowList { + ipAllowList, err = df.fetchIPAllowList(ctx, organization) + if err != nil { + df.logger.Warn("Skipping IP allow-list collection after GitHub API error", "org", organization, "error", err) + accumulatedErrors = errors.Join(accumulatedErrors, fmt.Errorf("failed to fetch IP allow-list. Please confirm a Classic token PAT is used to gather IP allowlist information: %w", err)) + ipAllowList = nil + } } return &GithubData{ @@ -142,8 +153,8 @@ func (df DataFetcher) FetchData(ctx context.Context, organization string) (*Gith Teams: allTeams, Members: allAdminMembers, SSO: ssoData, - IPAllowList: ipAllowList, - }, steps, nil + IPAllowList: &ipAllowList, + }, steps, accumulatedErrors } func (df DataFetcher) fetchSSO(ctx context.Context, organization string) (*OrgSSO, error) { @@ -233,7 +244,7 @@ func (df DataFetcher) fetchIPAllowList(ctx context.Context, organization string) } }` - var entries []IPAllowListEntry + entries := make([]IPAllowListEntry, 0) var after *string for { gqlQuery := graphqlRequest{ diff --git a/internal/data_test.go b/internal/data_test.go index 2ae5130..8028a02 100644 --- a/internal/data_test.go +++ b/internal/data_test.go @@ -10,6 +10,7 @@ import ( "github.com/google/go-github/v71/github" "github.com/hashicorp/go-hclog" + "github.com/open-policy-agent/opa/v1/rego" ) func testGithubClient(t *testing.T, handler http.Handler) (*github.Client, func()) { @@ -27,6 +28,142 @@ func testGithubClient(t *testing.T, handler http.Handler) (*github.Client, func( return client, server.Close } +func TestFetchDataReturnsErrorWhenOrganizationFetchFails(t *testing.T) { + client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "organization unavailable", http.StatusInternalServerError) + })) + defer cleanup() + + fetcher := NewDataFetcher(hclog.NewNullLogger(), client) + data, steps, err := fetcher.FetchData(context.Background(), "acme", false) + if err == nil { + t.Fatal("FetchData should return an error when the required organization endpoint fails") + } + if data != nil { + t.Fatalf("data = %#v, want nil", data) + } + if steps != nil { + t.Fatalf("steps = %#v, want nil", steps) + } +} + +func TestFetchDataSkipsOptionalCollectionErrors(t *testing.T) { + optionalRequests := make(map[string]int) + client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/orgs/acme": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"login":"acme","name":"Acme","url":"https://api.github.com/orgs/acme"}`)) + case "/orgs/acme/teams", "/orgs/acme/members", "/orgs/acme/sso", "/graphql": + optionalRequests[r.URL.Path]++ + http.Error(w, "optional data unavailable", http.StatusInternalServerError) + default: + t.Fatalf("unexpected request path %s", r.URL.Path) + } + })) + defer cleanup() + + fetcher := NewDataFetcher(hclog.NewNullLogger(), client) + data, steps, err := fetcher.FetchData(context.Background(), "acme", true) + if err == nil { + t.Fatal("FetchData should return accumulated errors when optional collections fail") + } + if data == nil { + t.Fatal("data should be returned when only optional collection fails") + } + if data.Settings.GetLogin() != "acme" { + t.Fatalf("organization login = %q, want acme", data.Settings.GetLogin()) + } + if len(data.Teams) != 0 { + t.Fatalf("len(Teams) = %d, want 0", len(data.Teams)) + } + if len(data.Members) != 0 { + t.Fatalf("len(Members) = %d, want 0", len(data.Members)) + } + if data.SSO != nil { + t.Fatalf("SSO = %#v, want nil", data.SSO) + } + if data.IPAllowList == nil { + t.Fatal("IPAllowList pointer should be set") + } + if *data.IPAllowList != nil { + t.Fatalf("IPAllowList = %#v, want nil slice for skipped collection", *data.IPAllowList) + } + if len(steps) == 0 { + t.Fatal("steps should still describe the collection activity") + } + for _, path := range []string{"/orgs/acme/teams", "/orgs/acme/members", "/orgs/acme/sso", "/graphql"} { + if optionalRequests[path] != 1 { + t.Fatalf("%s requests = %d, want 1", path, optionalRequests[path]) + } + } +} + +func TestFetchDataDoesNotCollectIPAllowListWhenDisabled(t *testing.T) { + graphqlRequests := 0 + client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/orgs/acme": + _, _ = w.Write([]byte(`{"login":"acme","name":"Acme","url":"https://api.github.com/orgs/acme"}`)) + case "/orgs/acme/teams", "/orgs/acme/members": + _, _ = w.Write([]byte(`[]`)) + case "/orgs/acme/sso": + http.NotFound(w, r) + case "/graphql": + graphqlRequests++ + t.Fatalf("GraphQL IP allow-list request should not be made when collection is disabled") + default: + t.Fatalf("unexpected request path %s", r.URL.Path) + } + })) + defer cleanup() + + fetcher := NewDataFetcher(hclog.NewNullLogger(), client) + data, steps, err := fetcher.FetchData(context.Background(), "acme", false) + if err != nil { + t.Fatalf("FetchData returned error: %v", err) + } + if data.IPAllowList == nil { + t.Fatal("IPAllowList pointer should be set") + } + if *data.IPAllowList != nil { + t.Fatalf("IPAllowList = %#v, want nil slice when collection is disabled", *data.IPAllowList) + } + encoded, err := json.Marshal(data) + if err != nil { + t.Fatalf("marshaling GithubData: %v", err) + } + var payload map[string]interface{} + if err := json.Unmarshal(encoded, &payload); err != nil { + t.Fatalf("unmarshaling GithubData: %v", err) + } + if value, ok := payload["ip_allow_list"]; !ok || value != nil { + t.Fatalf("JSON ip_allow_list = %#v, present = %v, want present null", value, ok) + } + evaluation, err := rego.New( + rego.Query("input.ip_allow_list"), + rego.Input(data), + ).Eval(context.Background()) + if err != nil { + t.Fatalf("evaluating OPA input: %v", err) + } + if len(evaluation) != 1 || len(evaluation[0].Expressions) != 1 { + t.Fatalf("OPA evaluation = %#v, want one null expression", evaluation) + } + if evaluation[0].Expressions[0].Value != nil { + t.Fatalf("OPA input.ip_allow_list = %#v, want null", evaluation[0].Expressions[0].Value) + } + if graphqlRequests != 0 { + t.Fatalf("GraphQL requests = %d, want 0", graphqlRequests) + } + for _, step := range steps { + if step.Title == "Get IP Allow-List" { + t.Fatal("IP allow-list collection step should not be present when collection is disabled") + } + } +} + func TestFetchSSOUsesRelativeURL(t *testing.T) { client, cleanup := testGithubClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { diff --git a/internal/eval_integration_test.go b/internal/eval_integration_test.go index 4dc9c82..29665af 100644 --- a/internal/eval_integration_test.go +++ b/internal/eval_integration_test.go @@ -21,7 +21,7 @@ func TestDataFetcher_FetchData(t *testing.T) { client: github.NewClient(nil).WithAuthToken(os.Getenv("GITHUB_TOKEN")), } - org, _, err := fetcher.FetchData(ctx, "compliance-framework") + org, _, err := fetcher.FetchData(ctx, "compliance-framework", false) if err != nil { t.Error(err) } diff --git a/main.go b/main.go index f6f8256..734c63c 100644 --- a/main.go +++ b/main.go @@ -15,8 +15,9 @@ import ( ) type PluginConfig struct { - Token string `mapstructure:"token"` - Organization string `mapstructure:"organization"` + Token string `mapstructure:"token"` + Organization string `mapstructure:"organization"` + CollectIPAllowList bool `mapstructure:"collect_ip_allow_list"` } type Validator interface { @@ -72,7 +73,7 @@ func (l *CompliancePlugin) Configure(req *proto.ConfigureRequest) (*proto.Config // In this method, you should save any configuration values to your plugin struct, so you can later // re-use them in PrepareForEval and Eval. config := &PluginConfig{} - err := mapstructure.Decode(req.GetConfig(), config) + err := mapstructure.WeakDecode(req.GetConfig(), config) if err != nil { l.logger.Error("Configuration cannot be decoded. Ensure the correct data has been passed.") return nil, err @@ -127,11 +128,16 @@ func (l *CompliancePlugin) Eval(request *proto.EvalRequest, apiHelper runner.Api dataFetcher := internal.NewDataFetcher(l.logger, l.githubClient) - data, collectSteps, err := dataFetcher.FetchData(ctx, l.config.Organization) - if err != nil { - return &proto.EvalResponse{ - Status: proto.ExecutionStatus_FAILURE, - }, fmt.Errorf("failed to fetch data: %w", err) + data, collectSteps, dataErr := dataFetcher.FetchData(ctx, l.config.Organization, l.config.CollectIPAllowList) + if dataErr != nil { + l.logger.Warn("Completed with non-fatal data collection errors", "error", dataErr) + // Continue with partial data - errors are accumulated but not fatal + // Policies will use skip_reason for fields that couldn't be fetched + if data == nil { + return &proto.EvalResponse{ + Status: proto.ExecutionStatus_FAILURE, + }, fmt.Errorf("failed to fetch data: %w", dataErr) + } } stepActivities := append(activities, &proto.Activity{ @@ -163,7 +169,11 @@ func (l *CompliancePlugin) Eval(request *proto.EvalRequest, apiHelper runner.Api Status: evalStatus, } - return resp, nil + if dataErr != nil { + resp.Status = proto.ExecutionStatus_FAILURE + } + + return resp, dataErr } func main() { diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..8f3b420 --- /dev/null +++ b/main_test.go @@ -0,0 +1,43 @@ +package main + +import ( + "testing" + + "github.com/compliance-framework/agent/runner/proto" + "github.com/hashicorp/go-hclog" +) + +func TestConfigureDefaultsCollectIPAllowListToFalse(t *testing.T) { + plugin := &CompliancePlugin{logger: hclog.NewNullLogger()} + + _, err := plugin.Configure(&proto.ConfigureRequest{ + Config: map[string]string{ + "token": "token", + "organization": "acme", + }, + }) + if err != nil { + t.Fatalf("Configure returned error: %v", err) + } + if plugin.config.CollectIPAllowList { + t.Fatal("CollectIPAllowList should default to false") + } +} + +func TestConfigureParsesCollectIPAllowList(t *testing.T) { + plugin := &CompliancePlugin{logger: hclog.NewNullLogger()} + + _, err := plugin.Configure(&proto.ConfigureRequest{ + Config: map[string]string{ + "token": "token", + "organization": "acme", + "collect_ip_allow_list": "true", + }, + }) + if err != nil { + t.Fatalf("Configure returned error: %v", err) + } + if !plugin.config.CollectIPAllowList { + t.Fatal("CollectIPAllowList should be true when configured") + } +}