Skip to content

Basic authorization added.#1808

Open
konstantin-a-malyshev wants to merge 1 commit into
aws:mainfrom
konstantin-a-malyshev:basic-auth
Open

Basic authorization added.#1808
konstantin-a-malyshev wants to merge 1 commit into
aws:mainfrom
konstantin-a-malyshev:basic-auth

Conversation

@konstantin-a-malyshev

Copy link
Copy Markdown

Basic authorisation added.

@kmcginnes

Copy link
Copy Markdown
Collaborator

Thanks for the contribution, @konstantin-a-malyshev! We really appreciate you putting this together.

Since this change deals with authentication and credentials, it'll need to go through some extra security validation before we can merge. That part tends to take a while, so apologies in advance for the slower turnaround.

A couple of questions to help us out:

  1. What's prompting the need for Basic auth, and how are you planning to use it?
  2. Which graph database are you connecting to? I see the tooltip mentions JanusGraph with SaslAndHMACAuthenticator. Is that your setup, or are there others?

And a few things we'll dig into during review (nothing you need to act on yet):

  • There aren't any tests for the new auth path. We usually ask for test coverage on changed code, and credential handling is one place we really want it.
  • The credentials end up stored client-side in plaintext (base64 is just encoding), so we'll want to think through the storage side of things, including whether the proxy might log the Authorization header.
  • The Dockerfile CRLF changes (the sed 's/\r//' lines) look unrelated to Basic auth. Were those needed for something, or can they come out into a separate PR?

Thanks again, and we'll keep you posted as the review moves along.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants