Skip to content

Commit

Permalink
fix(HMS-3185): fail with 403 on failed assume role
Browse files Browse the repository at this point in the history
AssumeRole is supposed to assume the role that client is giving us.
This fixes the error code when this operation fails.

It could have also been 401 as the role might be considered an input.
Given this is a permissions operation, we went with 403.
  • Loading branch information
ezr-ondrej committed Nov 30, 2023
1 parent 6456cf8 commit 2fdf214
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 54 deletions.
2 changes: 1 addition & 1 deletion internal/clients/http/ec2/ec2_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func getStsAssumedCredentials(ctx context.Context, arn string, region string) (*
})
if err != nil {
logger.Warn().Err(err).Msg("Cannot assume role")
return nil, fmt.Errorf("cannot assume role %w", err)
return nil, fmt.Errorf("cannot assume role: %w", err)
}

return output.Credentials, nil
Expand Down
29 changes: 26 additions & 3 deletions internal/clients/stubs/ec2_stub.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ package stubs
import (
"context"
"fmt"
"net/http"

"github.com/RHEnVision/provisioning-backend/internal/clients"
"github.com/RHEnVision/provisioning-backend/internal/clients/http"
httpClients "github.com/RHEnVision/provisioning-backend/internal/clients/http"
"github.com/RHEnVision/provisioning-backend/internal/models"
"github.com/RHEnVision/provisioning-backend/internal/ptr"
"github.com/aws/aws-sdk-go-v2/service/ec2/types"
"github.com/aws/smithy-go"
smithyhttp "github.com/aws/smithy-go/transport/http"
)

type ec2CtxKeyType int
Expand Down Expand Up @@ -42,7 +45,27 @@ func newEC2ServiceClientStubWithRegion(ctx context.Context, region string) (clie
return nil, nil
}

func newEC2CustomerClientStubWithRegion(ctx context.Context, _ *clients.Authentication, _ string) (si clients.EC2, err error) {
func newEC2CustomerClientStubWithRegion(ctx context.Context, auth *clients.Authentication, _ string) (clients.EC2, error) {
// For special case - testing invalid ARN errors
if auth.Payload == "arn:aws:iam::0000:role/test-invalid" {
err := &smithy.OperationError{
ServiceID: "STS",
OperationName: "AssumeRole",
Err: &smithyhttp.ResponseError{
Response: &smithyhttp.Response{
Response: &http.Response{
StatusCode: 403,
},
},
Err: &smithy.GenericAPIError{
Code: "AccessDenied",
Fault: smithy.FaultServer,
Message: fmt.Sprintf("User: %s is not authorized to perform: sts:AssumeRole on resource: %s", "<serviceAccount>", auth.Payload),
},
},
}
return nil, fmt.Errorf("cannot assume role: %w", err)
}
return getEC2StubFromContext(ctx)
}

Expand Down Expand Up @@ -85,7 +108,7 @@ func (mock *EC2ClientStub) GetPubkeyName(ctx context.Context, fingerprint string
return *key.KeyName, nil
}
}
return "", http.ErrPubkeyNotFound
return "", httpClients.ErrPubkeyNotFound
}

func (mock *EC2ClientStub) DeleteSSHKey(ctx context.Context, handle string) error {
Expand Down
37 changes: 23 additions & 14 deletions internal/clients/stubs/sources_stub.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,27 @@ func AddSource(ctx context.Context, provider models.ProviderType) (*clients.Sour
if err != nil {
return nil, err
}
return stub.addSource(ctx, provider)
switch provider {
case models.ProviderTypeAWS:
return stub.addAuth(ctx, clients.NewAuthentication("arn:aws:iam::230214684733:role/Test", provider))
case models.ProviderTypeAzure:
return stub.addAuth(ctx, clients.NewAuthentication("4b9d213f-712f-4d17-a483-8a10bbe9df3a", provider))
case models.ProviderTypeGCP:
return stub.addAuth(ctx, clients.NewAuthentication("[email protected]", provider))
case models.ProviderTypeUnknown, models.ProviderTypeNoop:
// not implemented
return nil, ErrNotImplemented
}

return nil, ErrNotImplemented
}

func AddAuth(ctx context.Context, authentication *clients.Authentication) (*clients.Source, error) {
stub, err := getSourcesClientStub(ctx)
if err != nil {
return nil, err
}
return stub.addAuth(ctx, authentication)
}

func getSourcesClient(ctx context.Context) (clients.Sources, error) {
Expand All @@ -54,24 +74,13 @@ func getSourcesClientStub(ctx context.Context) (si *SourcesClientStub, err error
return si, err
}

func (stub *SourcesClientStub) addSource(ctx context.Context, provider models.ProviderType) (*clients.Source, error) {
func (stub *SourcesClientStub) addAuth(ctx context.Context, authentication *clients.Authentication) (*clients.Source, error) {
id := strconv.Itoa(len(stub.sources) + 2) // starts at 2 as 1 is reserved - TODO migrate users of the implicit id = 1
source := &clients.Source{
ID: id,
Name: "source-" + id,
}
switch provider {
case models.ProviderTypeAWS:
stub.auths[id] = clients.NewAuthentication("arn:aws:iam::230214684733:role/Test", provider)
case models.ProviderTypeAzure:
stub.auths[id] = clients.NewAuthentication("4b9d213f-712f-4d17-a483-8a10bbe9df3a", provider)
case models.ProviderTypeGCP:
stub.auths[id] = clients.NewAuthentication("[email protected]", provider)
case models.ProviderTypeUnknown, models.ProviderTypeNoop:
// not implemented
return nil, ErrNotImplemented
}

stub.auths[id] = authentication
stub.sources = append(stub.sources, source)
return source, nil
}
Expand Down
7 changes: 6 additions & 1 deletion internal/payloads/error_payload.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import (
"strings"

"github.com/RHEnVision/provisioning-backend/internal/clients"

"github.com/RHEnVision/provisioning-backend/internal/usrerr"
"github.com/aws/smithy-go"

"github.com/RHEnVision/provisioning-backend/internal/logging"
"github.com/RHEnVision/provisioning-backend/internal/version"
Expand Down Expand Up @@ -194,6 +194,11 @@ func NewStatusError(ctx context.Context, message string, err error) *ResponseErr
}

func NewAWSError(ctx context.Context, message string, err error) *ResponseError {
var awsAPIErr *smithy.GenericAPIError
if errors.As(err, &awsAPIErr) && awsAPIErr.Code == "AccessDenied" {
message = fmt.Sprintf("AWS assume role failed: %s", message)
return NewResponseError(ctx, http.StatusForbidden, message, err)
}
message = fmt.Sprintf("AWS API error: %s", message)
return NewResponseError(ctx, http.StatusInternalServerError, message, err)
}
Expand Down
96 changes: 62 additions & 34 deletions internal/services/sources_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,52 +112,80 @@ func TestGetAzureSourceDetails(t *testing.T) {
})
}

func TestSourceUploadInfoFails(t *testing.T) {
t.Run("fails on AWS upload info for invalid id", func(t *testing.T) {
ctx := stubs.WithAccountDaoOne(context.Background())
ctx = identity.WithTenant(t, ctx)
ctx = clientStub.WithSourcesClient(ctx)
ctx = clientStub.WithEC2Client(ctx)

req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("/api/provisioning/sources/abcdef/upload_info"), nil)
require.NoError(t, err, "failed to create request")

rr := httptest.NewRecorder()
handler := http.HandlerFunc(GetSourceUploadInfo)
handler.ServeHTTP(rr, req)

require.Equal(t, http.StatusBadRequest, rr.Code, "Handler returned wrong status code")
func TestGetSourceUploadInfo(t *testing.T) {
t.Run("validates source ID", func(t *testing.T) {
t.Run("fails on AWS upload info for invalid id", func(t *testing.T) {
ctx := stubs.WithAccountDaoOne(context.Background())
ctx = identity.WithTenant(t, ctx)
ctx = clientStub.WithSourcesClient(ctx)
ctx = clientStub.WithEC2Client(ctx)

req, err := http.NewRequestWithContext(ctx, "GET", "/api/provisioning/sources/abcdef/upload_info", nil)
require.NoError(t, err, "failed to create request")

rr := httptest.NewRecorder()
handler := http.HandlerFunc(GetSourceUploadInfo)
handler.ServeHTTP(rr, req)

require.Equal(t, http.StatusBadRequest, rr.Code, "Handler returned wrong status code")
})

t.Run("fails on Azure upload info for invalid id", func(t *testing.T) {
ctx := stubs.WithAccountDaoOne(context.Background())
ctx = identity.WithTenant(t, ctx)
ctx = clientStub.WithSourcesClient(ctx)
ctx = clientStub.WithAzureClient(ctx)

req, err := http.NewRequestWithContext(ctx, "GET", "/api/provisioning/sources/{434FA35}/upload_info", nil)
require.NoError(t, err, "failed to create request")

rr := httptest.NewRecorder()
handler := http.HandlerFunc(GetSourceUploadInfo)
handler.ServeHTTP(rr, req)

require.Equal(t, http.StatusBadRequest, rr.Code, "Handler returned wrong status code")
})

t.Run("fails on GCP upload info for invalid id", func(t *testing.T) {
ctx := stubs.WithAccountDaoOne(context.Background())
ctx = identity.WithTenant(t, ctx)
ctx = clientStub.WithSourcesClient(ctx)
ctx = clientStub.WithGCPCCustomerClient(ctx)

req, err := http.NewRequestWithContext(ctx, "GET", "/api/provisioning/sources/{21234}/upload_info", nil)
require.NoError(t, err, "failed to create request")

rr := httptest.NewRecorder()
handler := http.HandlerFunc(GetSourceUploadInfo)
handler.ServeHTTP(rr, req)

require.Equal(t, http.StatusBadRequest, rr.Code, "Handler returned wrong status code")
})
})

t.Run("fails on Azure upload info for invalid id", func(t *testing.T) {
ctx := stubs.WithAccountDaoOne(context.Background())
ctx = identity.WithTenant(t, ctx)
ctx = clientStub.WithSourcesClient(ctx)
ctx = clientStub.WithAzureClient(ctx)

req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("/api/provisioning/sources/{434FA35}/upload_info"), nil)
require.NoError(t, err, "failed to create request")

rr := httptest.NewRecorder()
handler := http.HandlerFunc(GetSourceUploadInfo)
handler.ServeHTTP(rr, req)

require.Equal(t, http.StatusBadRequest, rr.Code, "Handler returned wrong status code")
})

t.Run("fails on GCP upload info for invalid id", func(t *testing.T) {
t.Run("fails with 403 on AWS invalid permissions or role", func(t *testing.T) {
ctx := stubs.WithAccountDaoOne(context.Background())
ctx = identity.WithTenant(t, ctx)
ctx = clientStub.WithSourcesClient(ctx)
ctx = clientStub.WithGCPCCustomerClient(ctx)
source, err := clientStub.AddAuth(ctx, &clients.Authentication{ProviderType: models.ProviderTypeAWS, Payload: "arn:aws:iam::0000:role/test-invalid"})
require.NoError(t, err, "failed to add source")

req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("/api/provisioning/sources/{21234}/upload_info"), nil)
rctx := chi.NewRouteContext()
ctx = context.WithValue(ctx, chi.RouteCtxKey, rctx)
rctx.URLParams.Add("ID", source.ID)
req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("/api/provisioning/sources/%s/upload_info", source.ID), nil)
require.NoError(t, err, "failed to create request")

rr := httptest.NewRecorder()
handler := http.HandlerFunc(GetSourceUploadInfo)
handler.ServeHTTP(rr, req)

require.Equal(t, http.StatusBadRequest, rr.Code, "Handler returned wrong status code")
var response payloads.ResponseError
err = json.NewDecoder(rr.Body).Decode(&response)
require.NoError(t, err, "failed to decode response body")

require.Equal(t, http.StatusForbidden, rr.Code, "Handler returned wrong status code")
require.Equal(t, "AWS assume role failed: unable to get AWS upload info", response.Message, "Response message is incorrect")
})
}
2 changes: 1 addition & 1 deletion scripts/rest_examples/source-1-upload-info.http
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @no-log
GET http://{{hostname}}:{{port}}/{{prefix}}/sources/1/upload_info HTTP/1.1
GET http://{{hostname}}:{{port}}/{{prefix}}/sources/515515/upload_info HTTP/1.1
Content-Type: application/json
X-Rh-Identity: {{identity}}

0 comments on commit 2fdf214

Please sign in to comment.