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

Openssl 1.1 support #7

Merged
merged 9 commits into from
May 31, 2018
Merged

Openssl 1.1 support #7

merged 9 commits into from
May 31, 2018

Conversation

xeroc
Copy link
Member

@xeroc xeroc commented Jan 5, 2018

This is an initial commit to include @nathanhourt's commit to support openssl-1.1.

However, it disables the blinding checks because they fail to compile. This needs to be fixed before merging.

@abitmore
Copy link
Member

As @cwyyprog mentioned in bitshares/bitshares-core#575, zaphoyd/websocketpp#599 would be useful for this ticket.

@xeroc
Copy link
Member Author

xeroc commented Feb 14, 2018

To fix this issue, I have placed a patched websocketpp into the bitshares project on github.
To apply the patch properly, you need to run

git submodule sync --recursive
git submodule update --init --recursive

//Edited by @abitmore: fixed typo in above commands.

@abitmore
Copy link
Member

@xeroc: the websocketpp fix is included in develop branch of upstream repo with this commit: zaphoyd/websocketpp@16d126e

@xeroc
Copy link
Member Author

xeroc commented Feb 14, 2018

@abitmore I didn't want to move to the develop branch and instead used tag 0.7.0 + cherry-picked LocutusOfBorg/websocketpp@1dd0711

@abitmore
Copy link
Member

abitmore commented Mar 7, 2018

@xeroc: we've updated the websocketpp sub module with another PR, please rebase and/or fix conflicts. (Update: actually anyone who have write permission can do this, if have time)

@xeroc
Copy link
Member Author

xeroc commented Mar 22, 2018

Rebased .. compiles here

nathanielhourt and others added 2 commits April 12, 2018 14:58
These changes should add support for openssl 1.1.0 while maintaining
compatibility with 1.0.2
@xeroc
Copy link
Member Author

xeroc commented Apr 12, 2018

One of the unit tests fails ...
anyone capable that knows how to fix that?
@jmjatlanta ?

@jmjatlanta
Copy link

jmjatlanta commented Apr 12, 2018

When I run blinding_test, I get Test setup error: test tree is empty
When I run all_tests I get:

unknown location(0): fatal error: in "fc_crypto/dh_test": memory access violation at address: 0x00000078: no mapping at fault address /home/jmjatlanta/Development/cpp/bitshares-fc/tests/crypto/dh_test.cpp(11): last checkpoint *** 1 failure is detected in the test module "AllTests"

Is that what you get?
My Environment:
Boost 1.63.0 and OpenSSL 1.1.0i-dev

I also tested with OpenSSL 1.0.1m, with the same results. So it is not an OpenSSL 1.1 issue. It seems to be the way we are initializing the diffie hellman object within this test. The actual underlying openssl dh object is null. I am trying to find out how and when it should be initialized.

After further testing, it looks like

ssl_dh dh;
should be ssl_dh dh(DH_new());

@xeroc
Copy link
Member Author

xeroc commented Apr 13, 2018

Thanks, unit tests no longer fail!

//edit: they don't fail with openssl-1.0, but do fail with openssl-1.1 :-(

@pmconrad
Copy link

I think we should rip out the blinding stuff.

The core uses it only insofar as it provides access through the crypto_api. The implementation (and tests) has been broken since this commit: cryptonomex@2b2dfc6 , so I'm pretty sure nobody's using it.

Fixed DH memory handling with openssl-1.1
@pmconrad
Copy link

I think we should rip out the blinding stuff.

@abitmore what do you think?

@abitmore
Copy link
Member

I agree.. I don't know much about crypto though.

@jmjatlanta
Copy link

kenCode was using some sort of blinding for his STEALTH coin. Should we ask him what he's relying on?

@pmconrad
Copy link

Like I said, the current implementation has been broken for >2 years. I'd be surprised if he's relying on that (and if he is he could as well maintain it himself...).

@xethyrion
Copy link

@pmconrad that implementation is partially relied upon and blind transfers do work using it so why rip out something that can benefit everyone?
-https://github.com/Agorise/bitshares-ui

@abitmore
Copy link
Member

abitmore commented Apr 26, 2018

@xethyrion blind transfer in BitShares does not rely on that, but on another module.

By the way, linking the UI repository here doesn't make sense, since UI doesn't use fc library. Even if it does, please link the actually code block that uses it but not the main url of the repository.

@xethyrion
Copy link

@abitmore worried about witnesses which do need to validate blind tx's... chris should comment here on that.

@abitmore
Copy link
Member

@xethyrion we need proofs. Why do you think it's needed?

@xethyrion
Copy link

@abitmore pinged chris, he's the expert on this, he says he'll look over here in an hour or two.

@christophersanborn
Copy link
Member

I'll look over in a bit whether we are relying on the blind code in question for our Blind/Stealth stuff. (Could someone provide a link to the specific code? @pmconrad?)

blind transfer in BitShares doesn't not rely on that, but on another module.

@abitmore - Interesting. I was unaware of a separate selection of blind code. What we are talking about here is distinct from what is used to validate Blind transactions on the network?

If I may ask, what was the original purpose of the blind code that we are now proposing to remove?

(If indeed it has been broken since 2015... then yeah presumably we are not relying on it. But just to emphasize: we HAVE conducted Blind transactions on the Main Net and we ARE developing features on top of it. And there is a non-zero amount of assets that are held in blinded balances. So, retaining the ability of witness nodes to validate blinded transactions is certainly critical.)

@pmconrad
Copy link

Of course I do not propose to remove the existing STEALTH feature from the blockchain backend.

This is a different kind of blinding. I implemented it myself in response to a bounty offered by BM, see https://bitsharestalk.org/index.php?topic=17315.0 .
The only reference to this in the core code is these two API calls: https://github.com/bitshares/bitshares-core/blob/master/libraries/app/api.cpp#L465-L477

@christophersanborn
Copy link
Member

Ah, cool. Thanks for the clarification @pmconrad!

@abitmore
Copy link
Member

@pmconrad so we need to remove the 2 API calls if this got removed from FC?

@pmconrad
Copy link

yes

@christophersanborn
Copy link
Member

This is a different kind of blinding. I implemented it myself in response to a bounty offered by BM, see https://bitsharestalk.org/index.php?topic=17315.0 .

@pmconrad — Interesting! I've read that whole thread before, a while back, and I was confused because I didn't see how Oleg's blind sig scheme quite fit in with the blind transfers. Now I see that it was meant for a different purpose!

Anyway, yeah, we're not using the Oleg Andreev scheme at all (blind_sign, etc.). We are only using the Pedersen commitment stuff (blind_sum, range_proof_sign, etc.), which of course needs to stay in order to validate blinded transfers.

@pmconrad
Copy link

@christophersanborn thanks for confirming!

@@ -1,6 +1,9 @@
#include <fc/crypto/dh.hpp>
#include <openssl/dh.h>

#if OPENSSL_VERSION_NUMBER >= 0x10100000L
#endif

Choose a reason for hiding this comment

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

Do these two lines do anything?

Choose a reason for hiding this comment

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

No, that's probably a leftover C&P template or sth like that.

Copy link

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Tested with

  • gcc-5, boost-1.60, openssl-1.0.2
  • gcc-6, boost-1.65.1, openssl-1.0.2
  • gcc-6, boost-1.65.1, openssl-1.1.0

With boost-1.65.1 the stacktrace unit tests fail, but I think that's unrelated.

All other known-working tests continue to work, replayed successfully + connected to mainnet.

@abitmore
Copy link
Member

How about gcc-5, boost-1.58, openssl-1.0.2 (Ubuntu 16.04 LTS default packages)?

@pmconrad
Copy link

gcc-5, boost-1.58, openssl-1.0.2 tested successfully too.

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.

8 participants