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-13359] move upstream basic auth to ee package #6669

Merged
merged 5 commits into from
Oct 27, 2024

Conversation

jeffy-mathew
Copy link
Contributor

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

User description

Description

This PR moves upstream basic auth implementations to ee package

Related Issue

Parent: https://tyktech.atlassian.net/browse/TT-13359
Subtask: https://tyktech.atlassian.net/browse/TT-13389

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, Other


Description

  • Refactored UpstreamBasicAuth to use a new AuthSource struct for better configuration management.
  • Implemented a new middleware for upstream basic authentication, including lifecycle and request processing methods.
  • Integrated the new middleware into the API processing chain.
  • Added EE-specific implementations and build constraints to separate enterprise features.

Changes walkthrough 📝

Relevant files
Enhancement
8 files
api_definitions.go
Refactor UpstreamBasicAuth to use AuthSource struct           

apidef/api_definitions.go

  • Changed HeaderName to Header in UpstreamBasicAuth.
  • Introduced AuthSource struct for auth configurations.
  • Added methods IsEnabled and AuthKeyName to AuthSource.
  • +24/-2   
    upstream.go
    Update UpstreamBasicAuth to use AuthSource in OAS               

    apidef/oas/upstream.go

  • Changed HeaderName to Header in UpstreamBasicAuth.
  • Updated Fill and ExtractTo methods to handle AuthSource.
  • +18/-5   
    middleware.go
    Implement Upstream Basic Auth Middleware                                 

    ee/middleware/upstreambasicauth/middleware.go

  • Added new middleware for upstream basic authentication.
  • Implemented methods for middleware lifecycle and request processing.
  • +76/-0   
    model.go
    Define Middleware Model and APISpec Structures                     

    ee/middleware/upstreambasicauth/model.go

  • Defined interfaces and structures for middleware dependencies.
  • Added APISpec struct for middleware configuration.
  • +48/-0   
    provider.go
    Implement Provider for Upstream Authentication                     

    ee/middleware/upstreambasicauth/provider.go

  • Implemented Provider for upstream authentication.
  • Added method to set request headers for authentication.
  • +26/-0   
    api_loader.go
    Integrate Upstream Basic Auth Middleware in API Loader     

    gateway/api_loader.go

    • Integrated new upstream basic auth middleware into API processing.
    +4/-1     
    mw_upstream_basic_auth.go
    Replace UpstreamBasicAuth with Noop for Non-EE Builds       

    gateway/mw_upstream_basic_auth.go

  • Replaced UpstreamBasicAuth with noopUpstreamBasicAuth.
  • Added build constraints for non-EE and non-dev environments.
  • +14/-54 
    mw_upstream_basic_auth_ee.go
    Add EE-Specific Upstream Basic Auth Middleware                     

    gateway/mw_upstream_basic_auth_ee.go

  • Added EE-specific implementation for upstream basic auth middleware.
  • +14/-0   
    Tests
    1 files
    mw_upstream_basic_auth_test.go
    Update Test Build Constraints for EE Middleware                   

    gateway/mw_upstream_basic_auth_test.go

    • Added build constraints for EE and dev environments.
    +2/-0     

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

    @buger
    Copy link
    Member

    buger commented Oct 24, 2024

    💔 The detected issue is not in one of the allowed statuses 💔

    Detected Status Open
    Allowed Statuses In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review ✔️

    Please ensure your jira story is in one of the allowed statuses

    Copy link
    Contributor

    github-actions bot commented Oct 24, 2024

    API Changes

    --- prev.txt	2024-10-25 14:26:59.613540649 +0000
    +++ current.txt	2024-10-25 14:26:52.941483202 +0000
    @@ -2416,9 +2416,9 @@
     	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.
    +	// Header holds the configuration for custom header name to be used for upstream basic authentication.
     	// Defaults to `Authorization`.
    -	HeaderName string `bson:"header_name" json:"header_name"`
    +	Header AuthSource `bson:"header" json:"header"`
     }
         UpstreamBasicAuth holds upstream basic authentication configuration.
     
    @@ -5252,9 +5252,8 @@
     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"`
    +	// Header contains configurations for the header value.
    +	Header *AuthSource `bson:"header,omitempty" json:"header,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.
    @@ -8184,6 +8183,87 @@
     }
         StreamsConfig represents a stream configuration.
     
    +# Package: ./ee/middleware/upstreambasicauth
    +
    +package upstreambasicauth // import "github.com/TykTechnologies/tyk/ee/middleware/upstreambasicauth"
    +
    +
    +CONSTANTS
    +
    +const (
    +	// ExtensionTykStreaming is the OAS extension for Tyk streaming.
    +	ExtensionTykStreaming = "x-tyk-streaming"
    +	StreamGCInterval      = 1 * time.Minute
    +)
    +
    +TYPES
    +
    +type APISpec struct {
    +	APIID string
    +	Name  string
    +	IsOAS bool
    +	OAS   oas.OAS
    +
    +	UpstreamAuth apidef.UpstreamAuth
    +}
    +    APISpec is a subset of gateway.APISpec for the values the middleware
    +    consumes.
    +
    +func NewAPISpec(id string, name string, isOasDef bool, oasDef oas.OAS, upstreamAuth apidef.UpstreamAuth) *APISpec
    +    NewAPISpec creates a new APISpec object based on the required inputs.
    +    The resulting object is a subset of `*gateway.APISpec`.
    +
    +type BaseMiddleware interface {
    +	model.LoggerProvider
    +}
    +    BaseMiddleware is the subset of BaseMiddleware APIs that the middleware
    +    uses.
    +
    +type Gateway interface {
    +	model.ConfigProvider
    +	model.ReplaceTykVariables
    +}
    +    Gateway is the subset of Gateway APIs that the middleware uses.
    +
    +type Middleware struct {
    +	Spec *APISpec
    +	Gw   Gateway
    +
    +	// Has unexported fields.
    +}
    +    Middleware implements upstream basic auth middleware.
    +
    +func NewMiddleware(gw Gateway, mw BaseMiddleware, spec *APISpec) *Middleware
    +    NewMiddleware returns a new instance of Middleware.
    +
    +func (m *Middleware) EnabledForSpec() bool
    +    EnabledForSpec checks if streaming is enabled on the config.
    +
    +func (m *Middleware) Init()
    +    Init initializes the middleware.
    +
    +func (m *Middleware) Logger() *logrus.Entry
    +    Logger returns a logger with middleware filled out.
    +
    +func (m *Middleware) Name() string
    +    Name returns the name for the middleware.
    +
    +func (m *Middleware) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
    +    ProcessRequest will handle upstream basic auth.
    +
    +type Provider struct {
    +	// Logger is the logger to be used.
    +	Logger *logrus.Entry
    +	// HeaderName is the header name to be used to fill upstream auth with.
    +	HeaderName string
    +	// AuthValue is the value of auth header.
    +	AuthValue string
    +}
    +    Provider implements upstream auth provider.
    +
    +func (u Provider) Fill(r *http.Request)
    +    Fill sets the request's HeaderName with AuthValue
    +
     # Package: ./gateway
     
     package gateway // import "github.com/TykTechnologies/tyk/gateway"
    @@ -11443,34 +11523,6 @@
         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 UpstreamBasicAuthProvider struct {
    -	// HeaderName is the header name to be used to fill upstream auth with.
    -	HeaderName string
    -	// AuthValue is the value of auth header.
    -	AuthValue string
    -}
    -    UpstreamBasicAuthProvider implements upstream auth provider.
    -
    -func (u UpstreamBasicAuthProvider) Fill(r *http.Request)
    -    Fill sets the request's HeaderName with AuthValue
    -
     type UpstreamOAuth struct {
     	*BaseMiddleware
     }

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    The methods IsEnabled and AuthKeyName in the AuthSource struct seem to be duplicated across different files. Consider creating a shared package or file to define these methods in one place to adhere to the DRY principle.

    Error Handling
    The Fill and ExtractTo methods in UpstreamBasicAuth struct do not handle potential nil pointer dereferences when accessing properties of api.Header. It's important to add nil checks before accessing these properties to prevent runtime panics.

    Logging Level
    The use of Info level for logging potential sensitive operations such as overwriting headers in Provider.Fill method might not be appropriate. Consider using Warning or Error level to highlight the importance of this action.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent potential nil pointer dereference by checking if Header is nil

    Consider checking if Header is nil before accessing its methods to prevent potential
    nil pointer dereference.

    apidef/api_definitions.go [820-821]

    -if !a.IsEnabled() {
    +if a == nil || !a.IsEnabled() {
         return ""
     }
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential nil pointer dereference, which is a critical issue that can cause runtime errors. By adding a nil check, the code becomes more robust and prevents unexpected crashes.

    8
    Add null check for api.Header to avoid nil pointer dereference

    Ensure that Header is properly initialized before calling Fill to avoid nil pointer
    dereference.

    apidef/oas/upstream.go [624-627]

     if u.Header == nil {
         u.Header = &AuthSource{}
     }
    -u.Header.Fill(api.Header.Enabled, api.Header.Name)
    +if api.Header != nil {
    +    u.Header.Fill(api.Header.Enabled, api.Header.Name)
    +}
    Suggestion importance[1-10]: 8

    Why: This suggestion prevents a potential nil pointer dereference by ensuring api.Header is not nil before calling its methods. This is crucial for preventing runtime errors and improving code stability.

    8
    Enhancement
    Prevent setting empty authentication headers

    Add a check to ensure AuthValue is not empty before setting the header, to avoid
    setting empty authentication headers.

    ee/middleware/upstreambasicauth/provider.go [25]

    -r.Header.Set(u.HeaderName, u.AuthValue)
    +if u.AuthValue != "" {
    +    r.Header.Set(u.HeaderName, u.AuthValue)
    +}
    Suggestion importance[1-10]: 7

    Why: The suggestion enhances the code by ensuring that empty authentication headers are not set, which can prevent potential security issues or misconfigurations.

    7
    Improve error handling by logging when UpstreamBasicAuth is not supported

    Log an error message when UpstreamBasicAuth is not supported instead of just
    returning false, to provide clearer debugging information.

    gateway/mw_upstream_basic_auth.go [22-24]

     if d.Spec.UpstreamAuth.BasicAuth.Enabled {
         d.Logger().Error("Upstream basic auth is supported only in Tyk Enterprise Edition")
    +    return false
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion improves error handling by ensuring that an error message is logged when UpstreamBasicAuth is not supported. This provides clearer debugging information, although it does not address a critical issue.

    6

    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-13359/ee-upstream-basic-authentication branch from a0b00f3 to 6d106df Compare October 25, 2024 13:14
    )

    // Provider implements upstream auth provider.
    type Provider struct {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This is a service struct which should have a constructor to pass the dependency (logger);
    Bit weird to have it structured O(1) like this

    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-13359/ee-upstream-basic-authentication branch from 6d106df to c26199c Compare October 25, 2024 13:35
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-13359/ee-upstream-basic-authentication branch from c26199c to 1660926 Compare October 25, 2024 14:25
    Copy link

    sonarcloud bot commented Oct 25, 2024

    Quality Gate Failed Quality Gate failed

    Failed conditions
    0.0% Coverage on New Code (required ≥ 80%)
    C Reliability Rating on New Code (required ≥ A)

    See analysis details on SonarCloud

    Catch issues before they fail your Quality Gate with our IDE extension SonarLint

    Copy link
    Contributor

    @titpetric titpetric left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Resolved some improvements feedback, nothing open should be a blocker.

    Follow up with #6672

    @jeffy-mathew jeffy-mathew merged commit f1b55b5 into master Oct 27, 2024
    34 of 40 checks passed
    @jeffy-mathew jeffy-mathew deleted the feat/TT-13359/ee-upstream-basic-authentication branch October 27, 2024 09:58
    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.

    3 participants