Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TT-13186/TT-13199] implement upstream basic authentication #6596

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jeffy-mathew
Copy link
Contributor

@jeffy-mathew jeffy-mathew commented Oct 1, 2024

User description

TT-13199
Summary Implement upstream basic authentication as a gateway middleware
Type Sub-task Sub-task
Status In Dev
Points N/A
Labels -

Description

Implement upstream basic authentication as a middleware.
Now users can configure upstream authentication using basic auth in

  • upstream_auth.basic_auth in Tyk classic API def.
  • upstream.authentication.basicAuth in Tyk OAS API def.

Related Issue

Parent: https://tyktech.atlassian.net/browse/TT-13186
Subtask: https://tyktech.atlassian.net/browse/TT-13199

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

Enhancement, Tests


Description

  • Implemented upstream basic authentication as a middleware, allowing users to configure authentication using basic auth in Tyk API definitions.
  • Added UpstreamAuth and UpstreamBasicAuth structs to manage authentication details.
  • Integrated upstream authentication into the OAS upstream configuration and reverse proxy handling.
  • Developed UpstreamBasicAuth middleware to handle basic authentication for upstream connections.
  • Added comprehensive tests to verify the functionality of the UpstreamBasicAuth middleware.

Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Add upstream authentication structures and methods             

apidef/api_definitions.go

  • Added UpstreamAuth struct to store upstream authentication
    information.
  • Introduced UpstreamBasicAuth struct for basic authentication details.
  • Added methods to check if upstream authentication is enabled.
  • +19/-0   
    upstream.go
    Integrate upstream authentication into OAS upstream configuration

    apidef/oas/upstream.go

  • Added Authentication field to Upstream struct for upstream
    authentication configuration.
  • Implemented methods to fill and extract authentication data.
  • +78/-0   
    ctx.go
    Add context management for upstream authentication             

    ctx/ctx.go

  • Added constants for upstream authentication header and value.
  • Implemented functions to set and get upstream authentication header
    and value.
  • +35/-0   
    api_loader.go
    Append UpstreamBasicAuth middleware to chain                         

    gateway/api_loader.go

    • Appended UpstreamBasicAuth middleware to the middleware chain.
    +2/-0     
    mw_upstream_basic_auth.go
    Implement UpstreamBasicAuth middleware for basic authentication

    gateway/mw_upstream_basic_auth.go

  • Implemented UpstreamBasicAuth middleware for basic authentication.
  • Added logic to inject basic auth info into request context.
  • +49/-0   
    reverse_proxy.go
    Integrate upstream authentication into reverse proxy         

    gateway/reverse_proxy.go

  • Added method to add authentication info to outgoing requests.
  • Integrated upstream authentication into request handling.
  • +16/-0   
    Tests
    mw_upstream_basic_auth_test.go
    Add tests for UpstreamBasicAuth middleware functionality 

    gateway/mw_upstream_basic_auth_test.go

  • Added tests for UpstreamBasicAuth middleware.
  • Verified basic authentication with default and custom headers.
  • +143/-0 
    http.go
    Add TestCases type for test management                                     

    test/http.go

    • Introduced TestCases type for managing multiple test cases.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @buger
    Copy link
    Member

    buger commented Oct 1, 2024

    I'm a bot and I 👍 this PR title. 🤖

    1 similar comment
    @buger
    Copy link
    Member

    buger commented Oct 1, 2024

    I'm a bot and I 👍 this PR title. 🤖

    Copy link
    Contributor

    github-actions bot commented Oct 1, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The implementation exposes sensitive information such as passwords in plain text within the codebase, which could lead to security vulnerabilities if the information is intercepted or improperly handled.

    ⚡ Recommended focus areas for review

    Security Concern
    The password is stored and transmitted in plain text which can lead to security vulnerabilities.

    Hardcoded Header
    The Authorization header is hardcoded, which might not be flexible for configurations that require custom headers.

    Copy link
    Contributor

    github-actions bot commented Oct 1, 2024

    API Changes

    --- prev.txt	2024-10-01 10:59:17.201977919 +0000
    +++ current.txt	2024-10-01 10:59:10.954984474 +0000
    @@ -853,7 +853,32 @@
     		},
             "detailed_tracing": {
                 "type": "boolean"
    -        }
    +        },
    +		"upstream_auth": {
    +			"type": "object",
    +			"properties": {
    +				"enabled": {
    +					"type": "boolean"
    +				},
    +				"basic_auth": {
    +					"type": "object",
    +					"properties": {
    +						"enabled": {
    +							"type": "boolean"
    +						},
    +						"username": {
    +							"type": "string"
    +						},
    +						"password": {
    +							"type": "string"
    +						},
    +						"header_name": {
    +							"type": "string"
    +						}
    +					}
    +				}
    +			}
    +    	}
         },
         "required": [
             "name",
    @@ -1034,6 +1059,9 @@
     	VersionName string `bson:"-" json:"-"`
     
     	DetailedTracing bool `bson:"detailed_tracing" json:"detailed_tracing"`
    +
    +	// UpstreamAuth stores information about authenticating against upstream.
    +	UpstreamAuth UpstreamAuth `bson:"upstream_auth" json:"upstream_auth"`
     }
         APIDefinition represents the configuration for a single proxied API and it's
         versions.
    @@ -1959,6 +1987,31 @@
     	MatchRegexp  *regexp.Regexp   `json:"-"`
     }
     
    +type UpstreamAuth struct {
    +	// Enabled enables upstream API authentication.
    +	Enabled bool `bson:"enabled" json:"enabled"`
    +	// BasicAuth holds the basic authentication configuration for upstream API authentication.
    +	BasicAuth UpstreamBasicAuth `bson:"basic_auth" json:"basic_auth"`
    +}
    +    UpstreamAuth holds the configurations related to upstream API
    +    authentication.
    +
    +func (u *UpstreamAuth) IsEnabled() bool
    +    IsEnabled checks if UpstreamAuthentication is enabled for the API.
    +
    +type UpstreamBasicAuth struct {
    +	// Enabled enables upstream basic authentication.
    +	Enabled bool `bson:"enabled" json:"enabled,omitempty"`
    +	// Username is the username to be used for upstream basic authentication.
    +	Username string `bson:"username" json:"username"`
    +	// Password is the password to be used for upstream basic authentication.
    +	Password string `bson:"password" json:"password"`
    +	// HeaderName is the custom header name to be used for upstream basic authentication.
    +	// Defaults to `Authorization`.
    +	HeaderName string `bson:"header_name" json:"header_name"`
    +}
    +    UpstreamBasicAuth holds upstream basic authentication configuration.
    +
     type UptimeTests struct {
     	CheckList []HostCheckObject `bson:"check_list" json:"check_list"`
     	Config    UptimeTestsConfig `bson:"config" json:"config"`
    @@ -4682,6 +4735,9 @@
     
     	// RateLimit contains the configuration related to API level rate limit.
     	RateLimit *RateLimit `bson:"rateLimit,omitempty" json:"rateLimit,omitempty"`
    +
    +	// Authentication contains the configuration related to upstream authentication.
    +	Authentication *UpstreamAuth `bson:"authentication,omitempty" json:"authentication,omitempty"`
     }
         Upstream holds configuration for the upstream server to which Tyk should
         proxy requests.
    @@ -4692,6 +4748,40 @@
     func (u *Upstream) Fill(api apidef.APIDefinition)
         Fill fills *Upstream from apidef.APIDefinition.
     
    +type UpstreamAuth struct {
    +	// Enabled enables upstream API authentication.
    +	Enabled bool `bson:"enabled" json:"enabled"`
    +	// BasicAuth holds the basic authentication configuration for upstream API authentication.
    +	BasicAuth *UpstreamBasicAuth `bson:"basicAuth,omitempty" json:"basicAuth,omitempty"`
    +}
    +    UpstreamAuth holds the configurations related to upstream API
    +    authentication.
    +
    +func (u *UpstreamAuth) ExtractTo(api *apidef.UpstreamAuth)
    +    ExtractTo extracts *UpstreamAuth into *apidef.UpstreamAuth.
    +
    +func (u *UpstreamAuth) Fill(api apidef.UpstreamAuth)
    +    Fill fills *UpstreamAuth from apidef.UpstreamAuth.
    +
    +type UpstreamBasicAuth struct {
    +	// Enabled enables upstream basic authentication.
    +	Enabled bool `bson:"enabled" json:"enabled"`
    +	// HeaderName is the custom header name to be used for upstream basic authentication.
    +	// Defaults to `Authorization`.
    +	HeaderName string `bson:"headerName" json:"headerName"`
    +	// Username is the username to be used for upstream basic authentication.
    +	Username string `bson:"username" json:"username"`
    +	// Password is the password to be used for upstream basic authentication.
    +	Password string `bson:"password" json:"password"`
    +}
    +    UpstreamBasicAuth holds upstream basic authentication configuration.
    +
    +func (u *UpstreamBasicAuth) ExtractTo(api *apidef.UpstreamBasicAuth)
    +    ExtractTo extracts *UpstreamBasicAuth into *apidef.UpstreamBasicAuth.
    +
    +func (u *UpstreamBasicAuth) Fill(api apidef.UpstreamBasicAuth)
    +    Fill fills *UpstreamBasicAuth from apidef.UpstreamBasicAuth.
    +
     type ValidateRequest struct {
     	// Enabled is a boolean flag, if set to `true`, it enables request validation.
     	Enabled bool `bson:"enabled" json:"enabled"`
    @@ -7200,8 +7290,24 @@
         API.
     
     func GetSession(r *http.Request) *user.SessionState
    +func GetUpstreamAuthHeader(r *http.Request) string
    +    GetUpstreamAuthHeader returns the header name to be used for upstream
    +    authentication.
    +
    +func GetUpstreamAuthValue(r *http.Request) string
    +    GetUpstreamAuthValue gets the auth header value to be used for upstream
    +    authentication.
    +
     func SetDefinition(r *http.Request, s *apidef.APIDefinition)
     func SetSession(r *http.Request, s *user.SessionState, scheduleUpdate bool, hashKey ...bool)
    +func SetUpstreamAuthHeader(r *http.Request, name string)
    +    SetUpstreamAuthHeader sets the header name to be used for upstream
    +    authentication.
    +
    +func SetUpstreamAuthValue(r *http.Request, name string)
    +    SetUpstreamAuthValue sets the auth header value to be used for upstream
    +    authentication.
    +
     
     TYPES
     
    @@ -7241,6 +7347,11 @@
     	// CacheOptions holds cache options required for cache writer middleware.
     	CacheOptions
     	OASDefinition
    +
    +	// UpstreamAuthHeader sets the header name to be used for upstream authentication.
    +	UpstreamAuthHeader
    +	// UpstreamAuthValue sets the value for upstream authentication.
    +	UpstreamAuthValue
     )
     # Package: ./dlpython
     
    @@ -10680,6 +10791,23 @@
         Enums representing the various statuses for a VersionInfo Path match during
         a proxy request
     
    +type UpstreamBasicAuth struct {
    +	*BaseMiddleware
    +}
    +    UpstreamBasicAuth is a middleware that will do basic authentication for
    +    upstream connections. UpstreamBasicAuth middleware is only supported in Tyk
    +    OAS API definitions.
    +
    +func (t *UpstreamBasicAuth) EnabledForSpec() bool
    +    EnabledForSpec returns true if the middleware is enabled based on API Spec.
    +
    +func (t *UpstreamBasicAuth) Name() string
    +    Name returns the name of middleware.
    +
    +func (t *UpstreamBasicAuth) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
    +    ProcessRequest will inject basic auth info into request context so that it
    +    can be used during reverse proxy.
    +
     type UptimeReportData struct {
     	URL          string
     	RequestTime  int64
    @@ -12180,6 +12308,8 @@
     	ControlRequest bool `json:",omitempty"`
     }
     
    +type TestCases []TestCase
    +
     type TransportOption func(*http.Transport)
         Options for populating a http.Transport
     

    Copy link
    Contributor

    github-actions bot commented Oct 1, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent nil pointer dereference by checking if the object is nil before accessing its method

    Check for potential nil pointer dereference on p.TykAPISpec.UpstreamAuth before
    calling IsEnabled() in addAuthInfo.

    gateway/reverse_proxy.go [1852]

    -if !p.TykAPISpec.UpstreamAuth.IsEnabled() {
    +if p.TykAPISpec.UpstreamAuth == nil || !p.TykAPISpec.UpstreamAuth.IsEnabled() {
         return
     }
    Suggestion importance[1-10]: 10

    Why: The suggestion effectively prevents a potential runtime error by adding a nil check before accessing the IsEnabled method. This change enhances the stability and reliability of the code by avoiding nil pointer dereferences.

    10
    Security
    Validate credentials to ensure they are not empty before processing

    Validate the Username and Password fields in UpstreamBasicAuth to ensure they are
    not empty before encoding to prevent misuse or errors.

    gateway/mw_upstream_basic_auth.go [44]

    +if basicAuthConfig.Username == "" || basicAuthConfig.Password == "" {
    +    return errors.New("username or password cannot be empty"), http.StatusBadRequest
    +}
     payload := base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", basicAuthConfig.Username, basicAuthConfig.Password)))
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential security issue by ensuring that the Username and Password fields are not empty before encoding. This validation prevents misuse and enhances the robustness of the authentication process.

    9
    Best practice
    Ensure JSON tags are consistent across struct fields

    Ensure that the UpstreamBasicAuth struct has proper JSON tags for the Enabled field
    to maintain consistency with other fields.

    apidef/api_definitions.go [780]

    -Enabled    bool   `bson:"enabled" json:"enabled,omitempty"`
    +Enabled    bool   `bson:"enabled" json:"enabled"`
    Suggestion importance[1-10]: 8

    Why: The suggestion improves consistency in JSON serialization by removing the omitempty tag from the Enabled field, aligning it with other fields in the struct. This change ensures that the Enabled field is always included in JSON output, which is crucial for maintaining consistent API behavior.

    8

    Copy link

    sonarcloud bot commented Oct 1, 2024

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants