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

feat: authnz service proxy #188

Draft
wants to merge 21 commits into
base: dev
Choose a base branch
from
Draft

feat: authnz service proxy #188

wants to merge 21 commits into from

Conversation

nmathew98
Copy link
Collaborator

issue: #171

https://discuss.elastic.co/t/kibana-7-5-disable-authentication/213935/7

Screen.Recording.2024-08-28.at.9.34.08.PM-2.mov

@nmathew98 nmathew98 requested a review from mwabe592 August 30, 2024 21:46
Copy link

Title

feat: authnz service proxy


User description

issue: #171

https://discuss.elastic.co/t/kibana-7-5-disable-authentication/213935/7

Screen.Recording.2024-08-28.at.9.34.08.PM-2.mov

PR Type

Enhancement, Tests, Configuration changes


Description

  • Defined environment constants and initialized environment variables using ferrite.
  • Set up HTTP server with middleware and routes for authentication and proxying.
  • Implemented Verify middleware for authentication, including functions for authentication and validation.
  • Added Dockerfile and configuration for development container.
  • Added build and deploy jobs for authnz-service-proxy Docker image in GitHub Actions workflows.
  • Updated Docker Compose and Nginx configuration to include and use authnz service.
  • Added test scaffolding for middleware functions.
  • Initialized Go module and dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
4 files
enums_env.go
Define environment constants for the service                         

authnz-service-proxy/enums/enums_env.go

  • Added Environment type and constants for production, development, and
    test.
  • +9/-0     
    main.go
    Initialize environment and set up HTTP server                       

    authnz-service-proxy/main.go

  • Initialized environment variables using ferrite.
  • Added configuration loading logic.
  • Set up HTTP server with middleware and routes.
  • +125/-0 
    data.go
    Define data structures for middleware                                       

    authnz-service-proxy/middleware/data.go

    • Defined ContextKey, Target, and Config types.
    +19/-0   
    middleware.go
    Implement authentication middleware and utilities               

    authnz-service-proxy/middleware/middleware.go

  • Implemented Verify middleware for authentication.
  • Added functions for authentication and validation.
  • Managed cookies for state and nonce.
  • +226/-0 
    Tests
    1 files
    middleware_test.go
    Add test scaffolding for middleware                                           

    authnz-service-proxy/middleware/middleware_test.go

    • Added test scaffolding for middleware functions.
    +26/-0   
    Configuration changes
    9 files
    build-services.yml
    Add build job for authnz-service-proxy Docker image           

    .github/workflows/build-services.yml

    • Added build job for authnz-service-proxy Docker image.
    +28/-0   
    deploy-services.yml
    Add deploy job for authnz-service-proxy Docker image         

    .github/workflows/deploy-services.yml

    • Added deploy job for authnz-service-proxy Docker image.
    +28/-0   
    Dockerfile
    Add Dockerfile for development container                                 

    authnz-service-proxy/.devcontainer/Dockerfile

    • Created Dockerfile for development container setup.
    +13/-0   
    devcontainer.json
    Add devcontainer configuration                                                     

    authnz-service-proxy/.devcontainer/devcontainer.json

    • Added configuration for development container.
    +49/-0   
    extensions.json
    Add recommended VSCode extensions                                               

    authnz-service-proxy/.vscode/extensions.json

    • Added recommended VSCode extensions.
    +8/-0     
    settings.json
    Add VSCode settings for Go development                                     

    authnz-service-proxy/.vscode/settings.json

    • Added VSCode settings for Go development.
    +8/-0     
    Dockerfile
    Add Dockerfile for authnz-service-proxy                                   

    authnz-service-proxy/Dockerfile

    • Created Dockerfile for authnz-service-proxy.
    +6/-0     
    docker-compose.proxy.yml
    Add authnz service to Docker Compose                                         

    telemetry/docker-compose.proxy.yml

    • Added authnz service to Docker Compose.
    +23/-2   
    nginx.conf
    Update Nginx configuration for authnz service                       

    telemetry/nginx/nginx.conf

    • Updated Nginx configuration to proxy requests to authnz service.
    +3/-3     
    Dependencies
    2 files
    go.mod
    Initialize Go module and dependencies                                       

    authnz-service-proxy/go.mod

    • Initialized Go module and dependencies.
    +22/-0   
    go.sum
    Add dependency checksums                                                                 

    authnz-service-proxy/go.sum

    • Added dependency checksums.
    +76/-0   

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🏅 Score: 85
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The use of WithSensitiveContent() for OIDC_CLIENT_SECRET and OIDC_CLIENT_ID is good, but ensure that these values are not logged or exposed in any way. Additionally, validate that the environment variables are securely managed and not hardcoded.

    ⚡ Key issues to review

    Possible Bug
    The function getProxyTarget does not handle the case where the header "AUTHNZ_PROXY_TARGET" is empty, which could lead to unexpected behavior.

    Code Smell
    The function isAuthenticationValid has multiple return points which can make it harder to read and maintain. Consider refactoring to reduce complexity.

    Copy link

    github-actions bot commented Aug 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add type assertion safety for the TargetUrl context value to prevent panics

    Consider handling the case where the TargetUrl context value is not a string to
    prevent potential panic due to type assertion failure.

    authnz-service-proxy/middleware/middleware.go [140]

    -targetUrl := r.Context().Value(TargetUrl).(string)
    +targetUrlValue := r.Context().Value(TargetUrl)
    +targetUrl, ok := targetUrlValue.(string)
    +if !ok {
    +    http.Error(w, "invalid target URL", http.StatusInternalServerError)
    +    return
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion adds necessary type assertion checks to prevent potential panics due to type assertion failures, improving the stability and reliability of the code.

    9
    Add validation for the PROXY_TARGET_URL to prevent nil dereference errors

    Ensure that the PROXY_TARGET_URL variable is properly validated to avoid potential
    nil dereference errors when accessing its value.

    authnz-service-proxy/main.go [43-46]

     PROXY_TARGET_URL = ferrite.
         String("PROXY_TARGET", "Proxy target url").
         Required()
    +if PROXY_TARGET_URL.Value() == "" {
    +    log.Fatal("PROXY_TARGET_URL cannot be empty")
    +}
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly adds validation to ensure PROXY_TARGET_URL is not empty, which helps prevent potential nil dereference errors. This is a significant improvement for robustness.

    8
    Best practice
    Log error messages before panicking to improve debugging context

    Consider logging the error messages before panicking to provide better context for
    debugging.

    authnz-service-proxy/middleware/middleware.go [88]

    +log.Printf("Error occurred: %v", err)
     panic(err)
     
    Suggestion importance[1-10]: 7

    Why: Logging error messages before panicking provides better context for debugging, which is a good practice. However, it is a minor improvement and not crucial for functionality.

    7

    Copy link

    🍹 preview on network/skulpture/shared-infrastructure

    Pulumi report
    Previewing update (shared-infrastructure)
    
    View Live: https://app.pulumi.com/skulpture/network/shared-infrastructure/previews/ae74cf4a-e469-450e-b765-47148bf2588d
    
    @ Previewing update........
    
    @ Previewing update................................................................
    pulumi:pulumi:Stack network-shared-infrastructure running 
    @ Previewing update....
    gcp:compute:Network shared-resources-network  
    digitalocean:index:DatabaseCluster shared-postgres  
    gcp:iam:WorkloadIdentityPool shared-resources-wif-pool  
    gcp:compute:Firewall allow-ssh  
    gcp:compute:Firewall allow-icmp  
    gcp:compute:Firewall allow-cloudflare  
    digitalocean:index:DatabaseDb shared-postgres-1  
    gcp:iam:WorkloadIdentityPoolProvider shared-resources  
    gcp:compute:InstanceIAMBinding shared-telemetry-compute-admin  
    gcp:compute:InstanceIAMBinding shared-rollout-compute-admin  
    gcp:compute:InstanceIAMBinding shared-authnz-compute-admin  
    pulumi:pulumi:Stack network-shared-infrastructure  
    Resources:
    12 unchanged
    
    

    Copy link

    🍹 preview on authnz/skulpture/shared-authnz

    Pulumi report
       Previewing update (shared-authnz)
    
    View Live: https://app.pulumi.com/skulpture/authnz/shared-authnz/previews/ea37e4a0-fb99-4f81-a0d2-9a217c19beb2
    
    @ Previewing update.......
    
    @ Previewing update.........................................................................
       pulumi:pulumi:Stack authnz-shared-authnz running 
    @ Previewing update....
       gcp:compute:Address shared-authnz  
       gcp:compute:Instance shared-authnz  
    ~  cloudflare:index:Record shared-authnz update [diff: +content]
       pulumi:pulumi:Stack authnz-shared-authnz  
    Resources:
       ~ 1 to update
       3 unchanged
    
       

    Copy link

    🍹 preview on rollout/skulpture/shared-rollout

    Pulumi report
       Previewing update (shared-rollout)
    
    View Live: https://app.pulumi.com/skulpture/rollout/shared-rollout/previews/13c51be0-5246-4eb9-824c-cab893aa2918
    
    @ Previewing update.........
    
    @ Previewing update.........................................................................
       pulumi:pulumi:Stack rollout-shared-rollout running 
    @ Previewing update....
       gcp:compute:Address shared-rollout  
    ~  cloudflare:index:Record shared-rollout update [diff: +content]
       gcp:compute:Instance shared-rollout  
       pulumi:pulumi:Stack rollout-shared-rollout  
    Resources:
       ~ 1 to update
       3 unchanged
    
       

    Copy link

    🍹 preview on telemetry/skulpture/shared-telemetry

    Pulumi report
       Previewing update (shared-telemetry)
    
    View Live: https://app.pulumi.com/skulpture/telemetry/shared-telemetry/previews/a5125555-393f-42d6-b483-3eef8c2edae0
    
    @ Previewing update.............
    
    @ Previewing update......................................................................
       pulumi:pulumi:Stack telemetry-shared-telemetry running 
    @ Previewing update.....
       gcp:compute:Address shared-telemetry  
    ~  cloudflare:index:Record shared-telemetry update [diff: +content]
       gcp:compute:Instance shared-telemetry  
       pulumi:pulumi:Stack telemetry-shared-telemetry  
    Resources:
       ~ 1 to update
       3 unchanged
    
       

    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.

    1 participant