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

Enable devlocal auth from flag and use environment checking #2081

Open
wants to merge 17 commits into
base: master
from

Conversation

2 participants
@chrisgilmerproj
Copy link
Contributor

commented May 2, 2019

Description

To support Load Testing in #1597 we need a way to turn on Devlocal Auth in experimental. This adds a flag that enables the devlocal auth but will check against the deployed environment and server names and fail if the environment is wrong.

Reviewer Notes

Could this be made more safe so we don't enable devlocal auth in production or staging? I am checking devlocal flag against:

  • The Environment flag (--environment)
  • All the Server Name Flags (my, office, tsp, and admin)

The combination here means multiple things need to fail in order to see this deployed in prod/staging.

Setup

Run the app. Try changing your .envrc.local vars for DEVLOCAL_AUTH.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

@chrisgilmerproj chrisgilmerproj self-assigned this May 2, 2019

@chrisgilmerproj chrisgilmerproj referenced this pull request May 2, 2019

Open

WIP - Try out load testing framework locust.io #1597

0 of 2 tasks complete
@codecov

This comment has been minimized.

Copy link

commented May 3, 2019

Codecov Report

Merging #2081 into master will decrease coverage by 0.31%.
The diff coverage is 29.55%.

@@            Coverage Diff             @@
##           master    #2081      +/-   ##
==========================================
- Coverage   58.78%   58.47%   -0.31%     
==========================================
  Files         240      241       +1     
  Lines       13888    13825      -63     
==========================================
- Hits         8164     8084      -80     
- Misses       4728     4750      +22     
+ Partials      996      991       -5
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Can I couple this to the hostname check? Not just dbEnv but also hostname and maybe another thing? The stronger the coupling the less likely this will happen.

@chrisgilmerproj chrisgilmerproj marked this pull request as ready for review May 7, 2019

chrisgilmerproj added some commits May 8, 2019

Update dbconn to use string constants and do better ssl mode checking…
…. Remove db env check from devlocal since it doesn't matter in this context of testing.
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@pjdufour-truss - I'm pretty sure this is done but you're the person I rely on for security checks. Is there another way to get the app deployed in AWS with devlocal auth enabled that doesn't possibly compromise prod/staging?


// CheckDevlocal validates the Devlocal command line flags
func CheckDevlocal(v *viper.Viper) error {
if devlocalAuthEnabled := v.GetBool(DevlocalAuthFlag); devlocalAuthEnabled {

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 10, 2019

Contributor

Declaring a variable seems to be redundant here.

DbEnvDevelopment string = "development"

// SSLModeDisable is the disable SSL Mode
SSLModeDisable string = "disable"

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 10, 2019

Contributor

I really like expanding our use of const values, but could we split the database ssl modes into a separate PR and jam it through without waiting on this bigger operational change?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 10, 2019

Author Contributor

I've moved it here: #2121

@@ -13,6 +13,8 @@ CONTAINER_CYPRESS=cypress
MILMOVEHOST=milmovelocal
OFFICEHOST=officelocal
TSPHOST=tsplocal
ADMINHOST=adminlocal

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss May 10, 2019

Contributor

These look like fixes for the admin app. Could we split these into their own PR?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj May 10, 2019

Author Contributor

I've moved it here: #2122. The reason I added these is that I'm checking the config against the admin hosts and that made it fail in the e2e tests. It's ok to have it separated though as it should have been added by the Admin team along with e2e tests.

@pjdufour-truss
Copy link
Contributor

left a comment

Added some comments inline. Would get good to break down this work into a few smaller PRs, if possible. Thanks!

chrisgilmerproj added some commits May 10, 2019

@chrisgilmerproj chrisgilmerproj requested a review from pjdufour-truss May 13, 2019

@chrisgilmerproj chrisgilmerproj force-pushed the cg_enable_devlocal_auth_with_environment_checking branch from 8be1238 to 7221888 May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.