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

Don't set http.sslCAInfo for schannel #172

Merged
merged 2 commits into from
Feb 16, 2018

Conversation

asalwa
Copy link
Contributor

@asalwa asalwa commented Feb 13, 2018

This addresses #1409

One strange thing in my fix may be the setting and unsetting of http.dummy variable. This is a work-around for Git config problem described here:
https://stackoverflow.com/questions/37147475/gitconfig-section-duplicates-everytime-i-set-a-preference
Without this trick the installer was leaving one empty [http] section. Everything else was working fine, but system config file looked ugly.

This is my first contribution to Git for Windows, please be forgiving :)

@dscho
Copy link
Member

dscho commented Feb 13, 2018

This is my first contribution to Git for Windows, please be forgiving :)

I always try ;-)

Thank you for your contribution!

One strange thing in my fix may be the setting and unsetting of http.dummy variable. This is a work-around for Git config problem described here:
https://stackoverflow.com/questions/37147475/gitconfig-section-duplicates-everytime-i-set-a-preference
Without this trick the installer was leaving one empty [http] section. Everything else was working fine, but system config file looked ugly.

If you don't mind, I would prefer a different approach:

  1. Change the order of the sslCAInfo and the sslBackend blocks in the first commit, explaining that this is a simple reordering so that the sslCAInfo can be unset in the next patch without introducing an ugly empty [http] section.

  2. In the second commit, guard the sslCAInfo call as you did, behind if not RdbCurlVariant[GC_WinSSL].Checked then begin [...] end;

What do you think?

@dscho
Copy link
Member

dscho commented Feb 13, 2018

(Oh, and if you agree, and do it that way, simply force-push to the same branch; This will update the PR automatically.)

@asalwa
Copy link
Contributor Author

asalwa commented Feb 13, 2018

Yes, I'm willing to follow your suggestion. Then nasty details come to the surface: I'd like to reorder sslCAInfo and sslBackend, but sslCAInfo is inside if FileExists(ProgramData+'\Git\config').
Can I just move sslBackend handling inside that "if"? Or should I first do some cleanup - maybe only commands touching ProgramData+'\Git\config' should be inside that "if"? And setting sslCAInfo in system config file should be outside?
Alternatively, we may also remove this check for FileExists(ProgramData+'\Git\config') at all. This file was just created a few lines above. And if creating the file failed, then it should be reported as error (LogError instead of Log if FileCopy(ExpandConstant('{tmp}\programdata-config.template'), ProgramData + '\Git\config', True) has failed.

@dscho
Copy link
Member

dscho commented Feb 14, 2018

Then nasty details come to the surface: I'd like to reorder sslCAInfo and sslBackend, but sslCAInfo is inside if FileExists(ProgramData+'\Git\config').

Personally, I would prefer the block

    {
        Configure http.sslBackend according to the user's choice.
    }

    if RdbCurlVariant[GC_WinSSL].Checked then begin
        Cmd:='schannel';
    end else begin
        Cmd:='openssl';
    end;
    if not Exec(AppDir+'\{#MINGW_BITNESS}\bin\git.exe','config --system http.sslBackend '+Cmd,
                AppDir,SW_HIDE,ewWaitUntilTerminated,i) then
        LogError('Unable to configure the HTTPS backend: '+Cmd);
    if HttpDummySet then begin
        if not Exec(AppDir+'\{#MINGW_BITNESS}\bin\git.exe','config --system --unset http.dummy',
                    AppDir,SW_HIDE,ewWaitUntilTerminated,i) then
            LogError('Unable to unset http.dummy');
    end;

to be moved just before the line if FileExists(ProgramData+'\Git\config') then begin (and add a comment before that).

What do you think?

Preparation for next commit which will unset variable in this section.
Without re-ordering we would trigger git config issue (empty section left).

Signed-off-by: Aleksander Salwa <asalwa@ra.rockwell.com>
@asalwa asalwa force-pushed the dont_set_sslCAInfo_for_schannel branch from d015e35 to 11473c2 Compare February 15, 2018 10:48
@asalwa
Copy link
Contributor Author

asalwa commented Feb 15, 2018

I've updated and pushed my branch according to your suggestions.

This way native Windows mechanism of certificates distribution
will be used.
This addresses #1409.

Signed-off-by: Aleksander Salwa <asalwa@ra.rockwell.com>
@asalwa asalwa force-pushed the dont_set_sslCAInfo_for_schannel branch from 11473c2 to 3bd10bf Compare February 15, 2018 10:58
@dscho
Copy link
Member

dscho commented Feb 16, 2018

Very nice! Thank you!

@dscho dscho merged commit 4713a15 into git-for-windows:master Feb 16, 2018
dscho added a commit that referenced this pull request Feb 16, 2018
When configuring HTTPS transport to use
Secure Channel, [we now refrain from configuring
`http.sslCAInfo`](#172).
This also helps Git LFS (which uses Git for Windows' private
`http.sslCAInfo` setting) to use the same credentials as `git fetch`
and `git push`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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