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

Middlewares as Go Plugins PoC [DO NOT MERGE] #6260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lonelycode
Copy link
Member

@lonelycode lonelycode commented May 7, 2024

User description

Description

Modified the mw_auth_key.go middleware to be compile-able as a Go Plugin, and have it loaded dynamically instead of as part of the monolith.

Motivation and Context

This is a PoC to see if it would be possible to re-factor our middleware functions as dynamic plugins. If we were able to ship plugins as .so files, it would be possible to ship fixes for specific middleware faster as we could simply provide patched .so files instead of full binaries.

Secondly, having middleware as plugins would make it easier to ship enterprise plugins against the gateway, so we could differentiate paid / non-paid functionality.

How This Has Been Tested

Badly.
This is a PoC, it compiles, and it successfully loads the plugin in a demo API Definition.

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)

PR Type

enhancement


Description

  • Exported several internal functions across various files for external use, enhancing modularity.
  • Integrated dynamic plugin loading for middleware, specifically the AuthKey middleware, to allow for more flexible deployments and updates.
  • Implemented the AuthKey middleware as a Go plugin, encapsulating its functionality and making it dynamically loadable.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
api.go
Export Internal Functions for External Use                             

gateway/api.go

  • Exported several internal functions to be accessible externally.
  • Added new exported functions for getting and setting request data,
    session, cache options, and more.
  • +29/-0   
    api_healthcheck.go
    Export Health Check Reporting Function                                     

    gateway/api_healthcheck.go

    • Exported the reportHealthValue function to allow external usage.
    +5/-0     
    api_loader.go
    Integrate Dynamic Plugin Loading for Middleware                   

    gateway/api_loader.go

    • Modified middleware loading to use dynamically loaded plugins.
    +3/-1     
    auth_manager.go
    Export Token Generation Function                                                 

    gateway/auth_manager.go

    • Exported the generateToken function for external use.
    +4/-0     
    handler_error.go
    Export Error Handling Functions                                                   

    gateway/handler_error.go

  • Exported the errorAndStatusCode function.
  • Commented out initialization of auth key errors.
  • +5/-1     
    log_helpers.go
    Export Key Obfuscation Function                                                   

    gateway/log_helpers.go

    • Exported the obfuscateKey function for external use.
    +4/-0     
    middleware.go
    Export Auth Token Retrieval Function                                         

    gateway/middleware.go

    • Exported the getAuthToken function for external use.
    +4/-0     
    mw_auth_key.go
    Refactor Auth Key Middleware to Load as Plugin                     

    gateway/mw_auth_key.go

  • Refactored to load middleware as a Go plugin.
  • Added plugin loading mechanism and error handling.
  • +276/-235
    mw_url_rewrite.go
    Export Variable Replacement Function                                         

    gateway/mw_url_rewrite.go

    • Exported the replaceTykVariables function for external use.
    +5/-0     
    mw_auth_key.go
    Implement AuthKey Middleware as Go Plugin                               

    plugins/authKey/mw_auth_key.go

  • Implemented the AuthKey middleware as a Go plugin.
  • Added error handling and session management within the plugin.
  • +278/-0 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    github-actions bot commented May 7, 2024

    API Changes

    --- prev.txt	2024-05-07 15:51:36.936092544 +0000
    +++ current.txt	2024-05-07 15:51:33.736044656 +0000
    @@ -1806,11 +1806,9 @@
     	Method string `bson:"method" json:"method"`
     }
     
    -type TykEvent = event.Event
    -    TykEvent is an alias maintained for backwards compatibility.
    +type TykEvent string // A type so we can ENUM event types easily, e.g. EventQuotaExceeded
     
    -type TykEventHandlerName = event.HandlerName
    -    TykEventHandlerName is an alias maintained for backwards compatibility.
    +type TykEventHandlerName string // A type for handler codes in API definitions
     
     type UDGGlobalHeader struct {
     	Key   string `bson:"key" json:"key"`
    @@ -1949,29 +1947,6 @@
     	ProxyOnError         bool       `bson:"proxy_on_error" json:"proxy_on_error"`
     }
     
    -type WebHookHandlerConf struct {
    -	// Disabled enables/disables this webhook.
    -	Disabled bool `bson:"disabled" json:"disabled"`
    -	// ID optional ID of the webhook, to be used in pro mode.
    -	ID string `bson:"id" json:"id"`
    -	// Name is the name of webhook.
    -	Name string `bson:"name" json:"name"`
    -	// The method to use for the webhook.
    -	Method string `bson:"method" json:"method"`
    -	// The target path on which to send the request.
    -	TargetPath string `bson:"target_path" json:"target_path"`
    -	// The template to load in order to format the request.
    -	TemplatePath string `bson:"template_path" json:"template_path"`
    -	// Headers to set when firing the webhook.
    -	HeaderList map[string]string `bson:"header_map" json:"header_map"`
    -	// The cool-down for the event so it does not trigger again (in seconds).
    -	EventTimeout int64 `bson:"event_timeout" json:"event_timeout"`
    -}
    -    WebHookHandlerConf holds configuration related to webhook event handler.
    -
    -func (w *WebHookHandlerConf) Scan(in any) error
    -    Scan scans WebHookHandlerConf from `any` in.
    -
     # Package: ./apidef/adapter
     
     package adapter // import "github.com/TykTechnologies/tyk/apidef/adapter"
    @@ -2933,43 +2908,6 @@
     func (et *EnforceTimeout) Fill(meta apidef.HardTimeoutMeta)
         Fill fills *EnforceTimeout from apidef.HardTimeoutMeta.
     
    -type Event struct {
    -	// Enabled enables the event handler.
    -	Enabled bool `json:"enabled" bson:"enabled"`
    -	// Type specifies the TykEvent that should trigger the event handler.
    -	Type event.Event `json:"type" bson:"type"`
    -	// Action specifies the action to be taken on the event trigger.
    -	Action event.Action `json:"action" bson:"action"`
    -	// ID is the ID of event handler in storage.
    -	ID string `json:"id,omitempty" bson:"id,omitempty"`
    -	// Name is the name of event handler
    -	Name string `json:"name,omitempty" bson:"name,omitempty"`
    -
    -	Webhook WebhookEvent `bson:"-" json:"-"`
    -}
    -    Event holds information about individual event to be configured on the API.
    -
    -func (e *Event) GetWebhookConf() (map[string]interface{}, error)
    -    GetWebhookConf converts Event.WebhookEvent to map[string]interface{}
    -    with apidef.WebHookHandlerConf structure for classic API definition
    -    compatibility.
    -
    -func (e *Event) MarshalJSON() ([]byte, error)
    -    MarshalJSON marshals Event as per Tyk OAS API definition contract.
    -
    -func (e *Event) UnmarshalJSON(in []byte) error
    -    UnmarshalJSON unmarshals Event as per Tyk OAS API definition contract.
    -
    -type Events []Event
    -    Events holds the list of events to be processed for the API.
    -
    -func (e *Events) ExtractTo(api *apidef.APIDefinition)
    -    ExtractTo extracts events to apidef.APIDefinition.
    -
    -func (e *Events) Fill(api apidef.APIDefinition)
    -    Fill fills Events from classic API definition. Currently only webhook events
    -    are supported.
    -
     type ExternalOAuth struct {
     	Enabled     bool `bson:"enabled" json:"enabled"` // required
     	AuthSources `bson:",inline" json:",inline"`
    @@ -3889,11 +3827,6 @@
     	//
     	// Tyk classic API definition: `detailed_tracing`
     	DetailedTracing *DetailedTracing `bson:"detailedTracing,omitempty" json:"detailedTracing,omitempty"`
    -
    -	// Events contains the configuration related to Tyk Events.
    -	//
    -	// Tyk classic API definition: `event_handlers`
    -	Events Events `bson:"events,omitempty" json:"events,omitempty"`
     }
         Server contains the configuration that sets Tyk up to receive requests from
         the client applications.
    @@ -4412,15 +4345,6 @@
         It is implemented to facilitate a smooth migration from deprecated fields
         that were previously used to represent the same data.
     
    -type WebhookEvent struct {
    -	URL          string            `json:"url" bson:"url"`
    -	Method       string            `json:"method" bson:"method"`
    -	Timeout      int64             `json:"timeout" bson:"timeout"`
    -	BodyTemplate string            `json:"bodyTemplate,omitempty" bson:"bodyTemplate,omitempty"`
    -	Headers      map[string]string `json:"headers,omitempty" bson:"headers,omitempty"`
    -}
    -    WebhookEvent stores the core information about a webhook event.
    -
     type XTykAPIGateway struct {
     	// Info contains the main metadata for the API definition.
     	Info Info `bson:"info" json:"info"` // required
    @@ -5375,7 +5299,6 @@
     func (c *Config) LoadIgnoredIPs()
     
     func (c *Config) SetEventTriggers(eventTriggers map[apidef.TykEvent][]TykEventHandler)
    -    SetEventTriggers sets events for backwards compatibility
     
     func (c *Config) StoreAnalytics(ip string) bool
     
    @@ -6870,40 +6793,26 @@
         other types. This is a go <1.13 cludge.
     
     const (
    -	// EventQuotaExceeded is an alias maintained for backwards compatibility.
    -	EventQuotaExceeded = event.QuotaExceeded
    -	// EventRateLimitExceeded is an alias maintained for backwards compatibility.
    -	EventRateLimitExceeded = event.RateLimitExceeded
    -
    -	// EventAuthFailure is an alias maintained for backwards compatibility.
    -	EventAuthFailure = event.AuthFailure
    -	// EventKeyExpired is an alias maintained for backwards compatibility.
    -	EventKeyExpired = event.KeyExpired
    -	// EventVersionFailure is an alias maintained for backwards compatibility.
    -	EventVersionFailure = event.VersionFailure
    -	// EventOrgQuotaExceeded is an alias maintained for backwards compatibility.
    -	EventOrgQuotaExceeded = event.OrgQuotaExceeded
    -	// EventOrgRateLimitExceeded is an alias maintained for backwards compatibility.
    -	EventOrgRateLimitExceeded = event.OrgRateLimitExceeded
    -	// EventTriggerExceeded is an alias maintained for backwards compatibility.
    -	EventTriggerExceeded = event.TriggerExceeded
    -	// EventBreakerTriggered is an alias maintained for backwards compatibility.
    -	EventBreakerTriggered = event.BreakerTriggered
    -	// EventBreakerTripped is an alias maintained for backwards compatibility.
    -	EventBreakerTripped = event.BreakerTripped
    -	// EventBreakerReset is an alias maintained for backwards compatibility.
    -	EventBreakerReset = event.BreakerReset
    -	// EventHOSTDOWN is an alias maintained for backwards compatibility.
    -	EventHOSTDOWN = event.HostDown
    -	// EventHOSTUP is an alias maintained for backwards compatibility.
    -	EventHOSTUP = event.HostUp
    -	// EventTokenCreated is an alias maintained for backwards compatibility.
    -	EventTokenCreated = event.TokenCreated
    -	// EventTokenUpdated is an alias maintained for backwards compatibility.
    -	EventTokenUpdated = event.TokenUpdated
    -	// EventTokenDeleted is an alias maintained for backwards compatibility.
    -	EventTokenDeleted = event.TokenDeleted
    +	EventQuotaExceeded        apidef.TykEvent = "QuotaExceeded"
    +	EventRateLimitExceeded    apidef.TykEvent = "RatelimitExceeded"
    +	EventAuthFailure          apidef.TykEvent = "AuthFailure"
    +	EventKeyExpired           apidef.TykEvent = "KeyExpired"
    +	EventVersionFailure       apidef.TykEvent = "VersionFailure"
    +	EventOrgQuotaExceeded     apidef.TykEvent = "OrgQuotaExceeded"
    +	EventOrgRateLimitExceeded apidef.TykEvent = "OrgRateLimitExceeded"
    +	EventTriggerExceeded      apidef.TykEvent = "TriggerExceeded"
    +	EventBreakerTriggered     apidef.TykEvent = "BreakerTriggered"
    +	EventBreakerTripped       apidef.TykEvent = "BreakerTripped"
    +	EventBreakerReset         apidef.TykEvent = "BreakerReset"
    +	EventHOSTDOWN             apidef.TykEvent = "HostDown"
    +	EventHOSTUP               apidef.TykEvent = "HostUp"
    +	EventTokenCreated         apidef.TykEvent = "TokenCreated"
    +	EventTokenUpdated         apidef.TykEvent = "TokenUpdated"
    +	EventTokenDeleted         apidef.TykEvent = "TokenDeleted"
     )
    +    Register new event types here, the string is the code used to hook at the
    +    Api Deifnititon JSON/BSON level
    +
     const (
     	MsgAuthFieldMissing                        = "Authorization field missing"
     	MsgApiAccessDisallowed                     = "Access to this API has been disallowed"
    @@ -6933,17 +6842,6 @@
     	DEFAULT_ORG_SESSION_EXPIRATION = int64(604800)
     )
     const (
    -	ErrAuthAuthorizationFieldMissing = "auth.auth_field_missing"
    -	ErrAuthKeyNotFound               = "auth.key_not_found"
    -	ErrAuthCertNotFound              = "auth.cert_not_found"
    -	ErrAuthCertExpired               = "auth.cert_expired"
    -	ErrAuthKeyIsInvalid              = "auth.key_is_invalid"
    -
    -	MsgNonExistentKey  = "Attempted access with non-existent key."
    -	MsgNonExistentCert = "Attempted access with non-existent cert."
    -	MsgInvalidKey      = "Attempted access with invalid key."
    -)
    -const (
     	KID       = "kid"
     	SUB       = "sub"
     	HMACSign  = "hmac"
    @@ -6991,18 +6889,11 @@
     const CoProcessDefaultKeyPrefix = "coprocess-data:"
         CoProcessDefaultKeyPrefix is used as a key prefix for this CP.
     
    -const EH_CoProcessHandler = event.CoProcessHandler
    -    EH_CoProcessHandler is used for event system, maintained here for backwards
    -    compatibility.
    -
    -const EH_JSVMHandler = event.JSVMHandler
    -    EH_JSVMHandler is aliased for backwards compatibility.
    +const EH_CoProcessHandler apidef.TykEventHandlerName = "cp_dynamic_handler"
    +    Constant for event system.
     
    -const (
    -	// EH_LogHandler is an alias maintained for backwards compatibility.
    -	// It is used to register log handler on an event.
    -	EH_LogHandler = event.LogHandler
    -)
    +const EH_JSVMHandler apidef.TykEventHandlerName = "eh_dynamic_handler"
    +const EH_LogHandler apidef.TykEventHandlerName = "eh_log_handler"
         The name for event handlers as defined in the API Definition JSON/BSON
         format
     
    @@ -7092,6 +6983,14 @@
     
     func CreateStandardPolicy() *user.Policy
     func CreateStandardSession() *user.SessionState
    +func CtxCheckLimits(r *http.Request) bool
    +func CtxGetData(r *http.Request) map[string]interface{}
    +    exported version
    +
    +func CtxGetSession(r *http.Request) *user.SessionState
    +func CtxSetData(r *http.Request, m map[string]interface{})
    +func CtxSetSession(r *http.Request, s *user.SessionState, scheduleUpdate bool, hashKey bool)
    +func CtxSetSpanAttributes(r *http.Request, mwName string, attrs ...otel.SpanAttribute)
     func DoCoprocessReload()
     func DurationToMillisecond(d time.Duration) float64
     func EncodeRequestToEvent(r *http.Request) string
    @@ -7099,6 +6998,7 @@
         it to base64 and store it in an Event object
     
     func EnsureTransport(host, protocol string) string
    +func ErrorAndStatusCode(errType string) (error, int)
     func GenerateTestBinaryData() (buf *bytes.Buffer)
     func GetAccessDefinitionByAPIIDOrSession(currentSession *user.SessionState, api *APISpec) (accessDef *user.AccessDefinition, allowanceScope string, err error)
     func GetTLSClient(cert *tls.Certificate, caCert []byte) *http.Client
    @@ -7116,6 +7016,7 @@
         will decode request body as json to map[string]string and adds the key/value
         pairs in r.Form.
     
    +func LoadAuthKeyMW() func(base *BaseMiddleware) TykMiddleware
     func LoadPoliciesFromDir(dir string) (map[string]user.Policy, error)
     func LoadPoliciesFromFile(filePath string) (map[string]user.Policy, error)
     func LoopingUrl(host string) string
    @@ -7148,9 +7049,11 @@
         PythonSetEnv sets PYTHONPATH, it's called before initializing the
         interpreter.
     
    +func ReportHealthValue(spec *APISpec, counter HealthPrefix, value string)
     func RevokeAllTokens(storage ExtendedOsinStorageInterface, clientId, clientSecret string) (int, []string, error)
     func RevokeToken(storage ExtendedOsinStorageInterface, token, tokenTypeHint string)
     func Start()
    +func StripBearer(token string) string
     func TestReq(t testing.TB, method, urlStr string, body interface{}) *http.Request
     func TestReqBody(t testing.TB, body interface{}) io.Reader
     func TykGetData(CKey *C.char) *C.char
    @@ -7320,16 +7223,6 @@
         ProcessRequest will run any checks on the request on the way through the
         system, return an error to have the chain fail
     
    -type AuthKey struct {
    -	*BaseMiddleware
    -}
    -    KeyExists will check if the key being used to access the API is in the
    -    request data, and then if the key is in the storage engine
    -
    -func (k *AuthKey) Name() string
    -
    -func (k *AuthKey) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
    -
     type BaseExtractor struct {
     	Config            *apidef.MiddlewareIdExtractor
     	IDExtractorConfig apidef.IDExtractorConfig
    @@ -7390,6 +7283,8 @@
         FireEvent is added to the BaseMiddleware object so it is available across
         the entire stack
     
    +func (t *BaseMiddleware) GetAuthToken(authType string, r *http.Request) (string, apidef.AuthConfig)
    +
     func (t *BaseMiddleware) Init()
     
     func (t *BaseMiddleware) Logger() (logger *logrus.Entry)
    @@ -7940,6 +7835,8 @@
     
     func (gw *Gateway) FireSystemEvent(name apidef.TykEvent, meta interface{})
     
    +func (gw *Gateway) GenerateToken(orgID, keyID string, customHashKeyFunction ...string) string
    +
     func (gw *Gateway) GetApiSpecsFromAccessRights(sess *user.SessionState) []*APISpec
         GetApiSpecsFromAccessRights from the session.AccessRights returns the
         collection of api specs
    @@ -7980,12 +7877,16 @@
     
     func (gw *Gateway) NotifyCurrentServerStatus()
     
    +func (gw *Gateway) ObfuscateKey(keyName string) string
    +
     func (gw *Gateway) ProcessOauthClientsOps(clients map[string]string)
         ProcessOauthClientsOps performs the appropriate action for the received
         clients it can be any of the Create,Update and Delete operations
     
     func (gw *Gateway) ProcessSingleOauthClientEvent(apiId, oauthClientId, orgID, event string)
     
    +func (gw *Gateway) ReplaceTykVariables(r *http.Request, in string, escape bool) string
    +
     func (gw *Gateway) RevokeAllTokensHandler(w http.ResponseWriter, r *http.Request)
     
     func (gw *Gateway) RevokeTokenHandler(w http.ResponseWriter, r *http.Request)
    @@ -9348,6 +9249,8 @@
         Statuses of the request, all are false-y except StatusOk and
         StatusOkAndIgnore
     
    +func CtxGetRequestStatus(r *http.Request) (stat RequestStatus)
    +
     type ResponseCacheMiddleware struct {
     	BaseTykResponseHandler
     	// Has unexported fields.
    @@ -9854,6 +9757,11 @@
     	Name() string
     }
     
    +type TykMiddlewareAsPlugin interface {
    +	TykMiddleware
    +	SetBase(*BaseMiddleware)
    +}
    +
     type TykOsinServer struct {
     	osin.Server
     	Config            *osin.ServerConfig
    @@ -10123,9 +10031,8 @@
     	WH_DELETE WebHookRequestMethod = "DELETE"
     	WH_PATCH  WebHookRequestMethod = "PATCH"
     
    -	// EH_WebHook is an alias maintained for backwards compatibility.
    -	// it is the handler to register a webhook event.
    -	EH_WebHook = event.WebHookHandler
    +	// Define the Event Handler name so we can register it
    +	EH_WebHook apidef.TykEventHandlerName = "eh_web_hook_handler"
     )
     type XPathExtractor struct {
     	BaseExtractor
    @@ -10252,6 +10159,45 @@
     
     func (t *TranslationFormatter) Format(entry *logrus.Entry) ([]byte, error)
     
    +# Package: ./plugins/authKey
    +
    +
    +
    +CONSTANTS
    +
    +const (
    +	ErrAuthAuthorizationFieldMissing = "auth.auth_field_missing"
    +	ErrAuthKeyNotFound               = "auth.key_not_found"
    +	ErrAuthCertNotFound              = "auth.cert_not_found"
    +	ErrAuthCertExpired               = "auth.cert_expired"
    +	ErrAuthKeyIsInvalid              = "auth.key_is_invalid"
    +
    +	MsgNonExistentKey  = "Attempted access with non-existent key."
    +	MsgNonExistentCert = "Attempted access with non-existent cert."
    +	MsgInvalidKey      = "Attempted access with invalid key."
    +)
    +
    +FUNCTIONS
    +
    +func AuthFailed(m gateway.TykMiddleware, r *http.Request, token string)
    +    TODO: move this method to base middleware?
    +
    +func GetPlugin(base *gateway.BaseMiddleware) gateway.TykMiddleware
    +
    +TYPES
    +
    +type AuthKey struct {
    +	*gateway.BaseMiddleware
    +}
    +    KeyExists will check if the key being used to access the API is in the
    +    request data, and then if the key is in the storage engine
    +
    +func (k *AuthKey) Name() string
    +
    +func (k *AuthKey) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
    +
    +func (k *AuthKey) SetBase(*gateway.BaseMiddleware)
    +
     # Package: ./regexp
     
     package regexp // import "github.com/TykTechnologies/tyk/regexp"

    Copy link

    github-actions bot commented May 7, 2024

    PR Description updated to latest commit (740aa9d)

    Copy link

    github-actions bot commented May 7, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    4, due to the complexity of the changes involving dynamic plugin loading, exporting functions, and potential security implications of handling plugins.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The LoadAuthKeyMW function in gateway/mw_auth_key.go uses a hardcoded path to load the plugin (/Users/martin/apps/tyk/plugins/authKey/authKey.so). This could lead to issues in environments where the path does not exist or is different.

    Performance Concern: Dynamic loading of plugins might introduce latency or performance overhead, especially if not properly managed or if the plugins are not optimized.

    🔒 Security concerns

    No explicit security vulnerabilities are introduced in the PR code itself, but the use of plugins could potentially lead to security risks if not properly sandboxed or if malicious plugins are inadvertently loaded.

    Code feedback:
    relevant filegateway/mw_auth_key.go
    suggestion      

    Consider making the plugin path configurable rather than hardcoding it. This can be achieved by using a configuration file or environment variables to specify the path, making the system more flexible and environment-independent. [important]

    relevant lineplug, err := plugin.Open("/Users/martin/apps/tyk/plugins/authKey/authKey.so")

    relevant filegateway/mw_auth_key.go
    suggestion      

    Implement error handling for the plugin loading process that does not terminate the program but allows the system to continue operating, possibly with reduced functionality. This can be crucial for maintaining service availability in production environments. [important]

    relevant lineos.Exit(1)

    relevant filegateway/mw_auth_key.go
    suggestion      

    Add logging details when a plugin fails to load or an incorrect type assertion occurs. This will help in debugging and maintaining the system by providing more context about the failure. [medium]

    relevant linefmt.Println("unexpected type from module symbol")

    relevant filegateway/mw_auth_key.go
    suggestion      

    Consider implementing a mechanism to verify the integrity and authenticity of the plugin files before loading them. This could involve checksum verification or digital signatures to ensure that the plugins have not been tampered with. [important]

    relevant lineplug, err := plugin.Open("/Users/martin/apps/tyk/plugins/authKey/authKey.so")

    Copy link

    github-actions bot commented May 7, 2024

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Add a nil check for the request pointer to prevent runtime errors.

    Consider checking for nil before dereferencing the r pointer in the CtxGetData function to
    prevent potential runtime panics.

    gateway/api.go [3085-3086]

     func CtxGetData(r *http.Request) map[string]interface{} {
    +  if r == nil {
    +    return nil
    +  }
       return ctxGetData(r)
     }
     
    Add a nil check for the middleware loader function to prevent runtime errors.

    Ensure that AuthKeyFunc is not nil before using it to avoid runtime panics.

    gateway/api_loader.go [390-391]

     AuthKeyFunc := LoadAuthKeyMW()
    +if AuthKeyFunc == nil {
    +  return errors.New("failed to load AuthKey middleware")
    +}
     AuthKey := AuthKeyFunc(baseMid)
     
    Enhancement
    Implement error handling to manage potential panics and improve robustness.

    Add error handling in CtxSetData to manage the panic from the underlying ctxSetData
    function, improving the function's robustness.

    gateway/api.go [3096-3097]

    -func CtxSetData(r *http.Request, m map[string]interface{}) {
    +func CtxSetData(r *http.Request, m map[string]interface{}) error {
    +  if m == nil {
    +    return errors.New("cannot set nil map in context")
    +  }
       ctxSetData(r, m)
    +  return nil
     }
     
    Enhance the wrapper function by adding error handling.

    Since CtxGetSession is a simple wrapper, consider adding error handling or context
    validation to provide additional value over the private function it wraps.

    gateway/api.go [3115-3116]

    -func CtxGetSession(r *http.Request) *user.SessionState {
    -  return ctxGetSession(r)
    +func CtxGetSession(r *http.Request) (*user.SessionState, error) {
    +  if r == nil {
    +    return nil, errors.New("request cannot be nil")
    +  }
    +  return ctxGetSession(r), nil
     }
     
    Enhance flexibility by configuring HTTP status codes through a configuration file.

    Replace the direct use of http.StatusUnauthorized and http.StatusForbidden with a more
    flexible configuration approach that allows changing these values without modifying the
    code. This can be achieved by adding these status codes to the configuration file and
    loading them during initialization.

    plugins/authKey/mw_auth_key.go [49-59]

    -Code:    http.StatusUnauthorized
    -Code:    http.StatusForbidden
    +Code:    config.UnauthorizedStatusCode
    +Code:    config.ForbiddenStatusCode
     
    Best practice
    Simplify the forwarding of variadic parameters.

    In CtxSetSpanAttributes, use variadic syntax directly in the function signature to avoid
    unnecessary unpacking and repacking of parameters.

    gateway/api.go [3405-3406]

     func CtxSetSpanAttributes(r *http.Request, mwName string, attrs ...otel.SpanAttribute) {
    -  ctxSetSpanAttributes(r, mwName, attrs...)
    +  ctxSetSpanAttributes(r, mwName, attrs)
     }
     
    Replace os.Exit with error handling to allow graceful error management.

    Replace the direct call to os.Exit(1) with a more graceful error handling approach, such
    as returning an error from the function. This allows the calling function to handle the
    error appropriately, possibly logging more context or attempting recovery steps.

    gateway/mw_auth_key.go [283-285]

     if err != nil {
         fmt.Println(err)
    -    os.Exit(1)
    +    return nil, err
     }
     
    Improve error handling by using structured errors for better traceability.

    Refactor the error handling in the validateSignature function to use a more structured
    approach, such as custom error types or error wrapping, to provide more context and
    improve error traceability.

    plugins/authKey/mw_auth_key.go [227]

    -return errors.New("internal server error"), http.StatusInternalServerError
    +return fmt.Errorf("validate signature failed: %w", err), http.StatusInternalServerError
     
    Maintainability
    Use a configuration value for the plugin path instead of hardcoding it.

    Consider using a configuration or constant for the plugin path instead of hardcoding it.
    This makes the code more flexible and easier to configure in different environments.

    gateway/mw_auth_key.go [281]

    -plug, err := plugin.Open("/Users/martin/apps/tyk/plugins/authKey/authKey.so")
    +plug, err := plugin.Open(config.PluginPath)
     
    Remove redundant function to simplify the codebase.

    Avoid duplicating the stripBearer function logic by removing the StripBearer function and
    directly using stripBearer where needed, or make stripBearer exported if it needs to be
    used outside the package.

    gateway/mw_auth_key.go [271-273]

    -func StripBearer(token string) string {
    -    return stripBearer(token)
    -}
    +// Remove the StripBearer function and use stripBearer directly.
     
    Evaluate the necessity of the ReplaceTykVariables wrapper function.

    Since ReplaceTykVariables is merely a wrapper for replaceTykVariables, consider if this
    additional layer of abstraction is necessary, or if it could be eliminated to simplify the
    codebase.

    gateway/mw_url_rewrite.go [258-260]

    -func (gw *Gateway) ReplaceTykVariables(r *http.Request, in string, escape bool) string {
    -    return gw.replaceTykVariables(r, in, escape)
    -}
    +// Consider removing ReplaceTykVariables if it's only a wrapper.
     
    Refactor repeated logic into a separate method to improve code maintainability.

    To improve the maintainability of the code, consider extracting the repeated logic for
    checking and setting session certificate into a separate method.

    plugins/authKey/mw_auth_key.go [148-153]

    -if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 {
    -    certLookup = certHash
    -    if session.Certificate != certHash {
    +func (k *AuthKey) updateSessionCertificate(r *http.Request, session *user.SessionState, certHash string) bool {
    +    if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 {
             session.Certificate = certHash
    -        updateSession = true
    +        return true
         }
    +    return false
     }
     
    Performance
    Optimize the bearer token stripping by using case-insensitive comparison.

    Refactor the stripBearer function to handle case insensitivity more efficiently by using
    strings.HasPrefix with a case-insensitive comparison instead of converting the entire
    string slice to uppercase.

    gateway/mw_auth_key.go [264-267]

    -if len(token) > 6 && strings.ToUpper(token[0:7]) == "BEARER " {
    +if len(token) > 6 && strings.EqualFold(token[0:7], "BEARER ") {
         return token[7:]
     }
     
    Simplify control flow in ProcessRequest for better readability and performance.

    Optimize the ProcessRequest method by reducing the number of conditional checks and
    simplifying the control flow to enhance readability and performance.

    plugins/authKey/mw_auth_key.go [105-111]

    -if gateway.CtxGetRequestStatus(r) == gateway.StatusOkAndIgnore {
    -    return nil, http.StatusOK
    -}
    -if ses := gateway.CtxGetSession(r); ses != nil && !gateway.CtxCheckLimits(r) {
    +if gateway.CtxGetRequestStatus(r) == gateway.StatusOkAndIgnore || (ses := gateway.CtxGetSession(r); ses != nil && !gateway.CtxCheckLimits(r)) {
         return nil, http.StatusOK
     }
     
    Bug
    Prevent potential runtime panics by checking slice length before access.

    Consider handling the case where r.TLS.PeerCertificates might be empty before accessing
    its elements to avoid potential runtime panics.

    plugins/authKey/mw_auth_key.go [124]

    -certHash = k.Spec.OrgID + crypto.HexSHA256(r.TLS.PeerCertificates[0].Raw)
    +if len(r.TLS.PeerCertificates) > 0 {
    +    certHash = k.Spec.OrgID + crypto.HexSHA256(r.TLS.PeerCertificates[0].Raw)
    +} else {
    +    // Handle error or fallback
    +}
     

    Copy link

    github-actions bot commented May 7, 2024

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    1 similar comment
    Copy link

    github-actions bot commented May 7, 2024

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    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.

    None yet

    1 participant