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

read extraheader from .git #2314

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

MarcelHillmann
Copy link

As developer I like to use our Github Enterprise, which is protected by an keycloak.

We use a tool to set an cookie to the git cli an the hub has use the same cookie too.

Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
@MarcelHillmann
Copy link
Author

MarcelHillmann commented Oct 19, 2019

Sorry I have no clue, why the cucumber test case is broken

Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Copy link
Owner

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Hi, this is an interesting feature. I'd like to enable you using hub with your Enterprise instance, but I wouldn't add this in this format yet, not until we clean up the code a bit and make some decisions about how hub.useragent is configured (I'd like to avoid keeping this in git config). If you're willing, let's iterate on this for now—I've added some thoughts below

github/client.go Outdated
if userAgent := os.Getenv("HUB_USERAGENT"); userAgent != "" {
UserAgent = userAgent
} else if userAgent, err := git.Config("hub.useragent"); err == nil {
UserAgent = userAgent
Copy link
Owner

Choose a reason for hiding this comment

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

Does your Enterprise instance also require setting a custom user agent string?

Copy link
Author

Choose a reason for hiding this comment

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

Hi,

our vhost is comparing the user-agent header, and accapt only "git/......".

I have verified it, a second ago.

github/client.go Outdated
@@ -970,6 +985,29 @@ func (client *Client) apiClient() *simpleClient {
return &simpleClient{
httpClient: httpClient,
rootUrl: apiRoot,
extraHeader: func(r *http.Header) {
if headers, err := git.ConfigAll(fmt.Sprintf("http.%s://%s.extraheader", client.Host.Protocol, client.Host.Host)); err == nil {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering whether it would be better to design this feature so that extraHeader is configured from the "outside" (being passed the list of extra headers), rather than making git requests here. I want to avoid the HTTP client being unnecessarily coupled to git config.

Copy link
Author

Choose a reason for hiding this comment

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

We have utility, to handel the process with KeyCloack as IDP.
This tool is writing the git/extraHeader for the git cli workflow.

https://github.com/git/git/blob/042ed3e048af08014487d19196984347e3be7d1c/Documentation/config/http.txt#L52

I could refactor this function, that it is call once and only the "r" manipulation is running

github/client.go Outdated Show resolved Hide resolved
github/client.go Outdated

if len(cookies) > 0 && httpClient.Jar == nil {
httpClient.Jar, _ = cookiejar.New(&cookiejar.Options{PublicSuffixList: publicsuffix.List})
httpClient.Jar.SetCookies(url, cookies)
Copy link
Owner

Choose a reason for hiding this comment

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

Is keeping a cookie jar really necessary for API requests to your Enterprise instance?

Copy link
Author

Choose a reason for hiding this comment

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

The dataflow is:

  1. Nginx
  2. 302: redirect to KeyCloack
    KeyCloack is forwarding to internal server with SessionID and traceIds
  3. Client is requesting again the GHE behind Nginx
  4. Nginx is passing to GHE

So there are 2 or 3 round trips with a bunch of cookies

github/http.go Outdated Show resolved Hide resolved
@mislav mislav added the feature label Oct 21, 2019
MarcelHillmann and others added 14 commits January 8, 2020 11:11
# Conflicts:
#	github/client.go
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
Signed-off-by: Marcel <marcel@mahillmann.de>
MarcelHillmann and others added 2 commits January 26, 2020 21:21
Signed-off-by: Marcel <marcel@mahillmann.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants