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

md4: use init/update/final functions in Secure Transport #4979

Closed
wants to merge 1 commit into from

Conversation

@nickzman
Copy link
Collaborator

@nickzman nickzman commented Feb 25, 2020

No description provided.

@nickzman nickzman requested a review from captain-caveman2k Feb 25, 2020
@captain-caveman2k
Copy link
Member

@captain-caveman2k captain-caveman2k commented Feb 25, 2020

It looks good Nick,

My only question is md5.c and tool_metalink.c use the following pre-processor check:

#elif (defined(__MAC_OS_X_VERSION_MAX_ALLOWED) && \
              (__MAC_OS_X_VERSION_MAX_ALLOWED >= 1040)) || \
      (defined(__IPHONE_OS_VERSION_MAX_ALLOWED) && \
              (__IPHONE_OS_VERSION_MAX_ALLOWED >= 20000))

whilst we use #elif defined(USE_SECTRANSP) here. Is there any benefit to either?

@jay
Copy link
Member

@jay jay commented Feb 25, 2020

Before you send this upstream please make sure to rebase on upstream/master (ie no merge commits) and then amend the commit message to add a separate section with a reference to the PR and any issues it solves, for example

md4: use init/update/final functions in Secure Transport

Usually an explanation here.

Fixes #xxxx
Closes #4979

If you need a refresher please refer to push-access-guidelines

@nickzman
Copy link
Collaborator Author

@nickzman nickzman commented Feb 26, 2020

@captain-caveman2k, I'll add that preprocessor macro, though I'll be very surprised if anyone still targets macOS 10.3 or earlier anymore, since it's almost 17 years old at this point.

@jay, I'll edit that. Is it okay if I use the "rebase & merge" button on Github, or does that do something undesired? You can tell I don't do this often...

@jay
Copy link
Member

@jay jay commented Feb 26, 2020

Is it okay if I use the "rebase & merge" button on Github,

Unfortunately no, this is covered in the guidelines, as far as I know the only way to get the correct attribution (ie committer remains committer) is by pushing from the command line. In this case that would look like:

git checkout nickzman/CC_MD4
git fetch upstream
git rebase upstream/master
git checkout master
git merge --ff-only nickzman/CC_MD4
gitk --all &    # check the final product
# amend if it isn't exactly what you expected, then check again
git push upstream

I'm assuming you use the name upstream for curl/curl, for example git remote -v may show

origin  git@github.com:nickzman/curl.git (fetch)
origin  git@github.com:nickzman/curl.git (push)
upstream        git@github.com:curl/curl.git (fetch)
upstream        git@github.com:curl/curl.git (push)
nickzman added a commit to nickzman/curl that referenced this pull request Feb 26, 2020
We can use CC_MD4_Init/Update/Final without having to allocate memory
directly.

Closes curl#4979
@nickzman nickzman force-pushed the nickzman:nickzman/CC_MD4 branch from ca38e76 to 27f822c Feb 26, 2020
@captain-caveman2k
Copy link
Member

@captain-caveman2k captain-caveman2k commented Feb 26, 2020

I'll add that preprocessor macro, though I'll be very surprised if anyone still targets macOS 10.3 or earlier anymore, since it's almost 17 years old at this point.

Do we still need the USE_SECTRANSP in there?

The main reason for asking is md5.c doesn't and I'm looking at your changes here for guidance in my sha256.c changes (over in #4956).

@nickzman
Copy link
Collaborator Author

@nickzman nickzman commented Feb 27, 2020

Actually, no, we really don't. It can run on macOS and iOS independently of the Security framework.

We can use CC_MD4_Init/Update/Final without having to allocate memory
directly.

Closes #4979
@nickzman nickzman force-pushed the nickzman:nickzman/CC_MD4 branch from 27f822c to f23dd5b Feb 28, 2020
@nickzman
Copy link
Collaborator Author

@nickzman nickzman commented Mar 1, 2020

Okay, I'm still trying to merge this from the command line, and I'm not having much luck. Here's what I'm doing:

% git remote add upstream git@github.com:curl/curl.git
% git checkout nickzman/CC_MD4
% git fetch upstream
% git rebase upstream/master
% git checkout master
% git merge --ff-only nickzman/CC_MD4
(review the results in SourceTree, then...)
% git push upstream
ERROR: Permission to curl/curl.git denied to nickzman.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I can type ssh git@github.com and get confirmation that my SSH key is working. Does some repository administrator need to add my key to the project independently of my settings? Because that's the only reason why this would fail, so far as I know...

@jay
Copy link
Member

@jay jay commented Mar 2, 2020

That's weird and I don't know what that is. You have write access and 2FA enabled so there should not be anything stopping you.

Capture

Try git remote -v see what it shows, also try GIT_TRACE=1 git ...

Does some repository administrator need to add my key to the project independently of my settings?

Not as far as I know. The ssh key should be the ssh key in the settings on your account.

Sometimes github has intermittent issues.

@nickzman
Copy link
Collaborator Author

@nickzman nickzman commented Mar 2, 2020

I'll try again later, but everything looks correct to me...

% git remote -v
origin	git@github.com:nickzman/curl.git (fetch)
origin	git@github.com:nickzman/curl.git (push)
upstream	git@github.com:curl/curl.git (fetch)
upstream	git@github.com:curl/curl.git (push)
% setenv GIT_TRACE 1
% git push upstream
18:38:20.075280 exec-cmd.c:139          trace: resolved executable path from Darwin stack: /Applications/Xcode.app/Contents/Developer/usr/bin/git
18:38:20.075807 exec-cmd.c:236          trace: resolved executable dir: /Applications/Xcode.app/Contents/Developer/usr/bin
18:38:20.076242 git.c:419               trace: built-in: git push upstream
18:38:20.081102 run-command.c:643       trace: run_command: unset GIT_PREFIX; ssh git@github.com 'git-receive-pack '\''curl/curl.git'\'''
ERROR: Permission to curl/curl.git denied to nickzman.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
% ssh git@github.com
PTY allocation request failed on channel 0
Hi nickzman! You've successfully authenticated, but GitHub does not provide shell access.
Connection to github.com closed.
@bagder
Copy link
Member

@bagder bagder commented Mar 2, 2020

I pushed it!

@bagder bagder closed this in 0b337ec Mar 2, 2020
nickzman added a commit to nickzman/curl that referenced this pull request Mar 27, 2020
We can use CC_MD4_Init/Update/Final without having to allocate memory
directly.

Closes curl#4979
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.