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

Switch to MbedTLS #447

Merged
merged 4 commits into from Jan 21, 2017
Merged

Switch to MbedTLS #447

merged 4 commits into from Jan 21, 2017

Conversation

malmaud
Copy link
Contributor

@malmaud malmaud commented Aug 14, 2016

Since Julia ships with MbedTLS binaries now, I'm trying to switch over packages from Nettle.

@malmaud
Copy link
Contributor Author

malmaud commented Aug 14, 2016

As far as I can tell, the AppVeyor failure doesn't seem related to this PR.

@stevengj
Copy link
Member

Julia 0.4 has this?

@malmaud
Copy link
Contributor Author

malmaud commented Aug 14, 2016

mbedtls doesn't ship with .4, but MbedTLS will still build(/binaries will be fetched) fine on .4.

@stevengj
Copy link
Member

Yeah, looks like a separate AppVeyor problem (with Conda).

function hmac(s1,s2,s3,s4)
if isempty(hmacstate)
if !isdefined(hmacstate, :x)
Copy link
Member

Choose a reason for hiding this comment

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

Julia should really have an isdefined(x::Ref) = isdefined(x, :x) method...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It really should.

@stevengj
Copy link
Member

Bump.

@tkelman
Copy link
Contributor

tkelman commented Sep 7, 2016

what is wrong with nettle exactly?

@malmaud
Copy link
Contributor Author

malmaud commented Sep 7, 2016

It's just one more binary dependency that can cause trouble. With mbedtls
bundled with Julia now, it seems the simplest.
On Wed, Sep 7, 2016 at 3:56 PM Tony Kelman notifications@github.com wrote:

what is wrong with nettle exactly?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#447 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA8SvbhK1Z_SBoEgbOAHGWZRMtf3Zuotks5qnxb1gaJpZM4JjzsY
.

@tkelman
Copy link
Contributor

tkelman commented Sep 7, 2016

except that mbedtls.jl doesn't use the bundled library yet, so you're trading one download for another.

@malmaud
Copy link
Contributor Author

malmaud commented Sep 7, 2016

It's just a matter of getting that PR in

On Wed, Sep 7, 2016 at 4:13 PM Tony Kelman notifications@github.com wrote:

except that mbedtls.jl doesn't use the bundled library yet, so you're
trading one download for another.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#447 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA8SvUYKmNxSNH3fj1JcnDhPLRSAJmAbks5qnxrygaJpZM4JjzsY
.

@tkelman
Copy link
Contributor

tkelman commented Sep 7, 2016

by "in" you mean "working" right? ref JuliaLang/MbedTLS.jl#66

@malmaud
Copy link
Contributor Author

malmaud commented Jan 19, 2017

I believe MbedTLS now indeed correctly uses the pre-shipped binaries, so time to revisit this.

@tkelman
Copy link
Contributor

tkelman commented Jan 19, 2017

only on Julia 0.5

@malmaud
Copy link
Contributor Author

malmaud commented Jan 19, 2017

Still, MbedTLS will just build/download as a normal external dependency on .4 as Nettle does now. Or were you thinking on Julia <.5 IJulia would still use Nettle? For that, I guess we'd to tag a minor version of IJulia that bumps the Julia requirement to .5.

@tkelman
Copy link
Contributor

tkelman commented Jan 19, 2017

That's probably worth doing when making this change, rather than trying to worry about one more permutation in which we need to keep things working. If Nettle works fine on 0.4, probably better to leave it be.

@@ -1,5 +1,5 @@
julia 0.4
Copy link
Contributor

@tkelman tkelman Jan 20, 2017

Choose a reason for hiding this comment

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

also in travis and appveyor, if stevengj agrees about now (well, after #502 gets merged and tagged first) being an okay time to drop 0.4

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think dropping 0.4 should be fine.

@malmaud
Copy link
Contributor Author

malmaud commented Jan 20, 2017

@stevengj I think this is good to go now

@stevengj stevengj merged commit 4d693e1 into master Jan 21, 2017
@stevengj stevengj deleted the use_mbedtls branch January 21, 2017 03:25
stevengj added a commit that referenced this pull request Jan 21, 2017
@@ -64,7 +64,7 @@ function init(args)
if signature_scheme[1] != "hmac" || length(signature_scheme) != 2
error("unrecognized signature_scheme $signature_scheme")
end
push!(hmacstate, HMACState(signature_scheme[2], profile["key"]))
hmacstate[] = MbedTLS.MD(MbedTLS.MD_SHA256, profile["key"])
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that this assumes signature_scheme[2] == "sha256"

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 7c8fe35

@malmaud
Copy link
Contributor Author

malmaud commented Jan 23, 2017 via email

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.

None yet

3 participants