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

Send payments to emails, urls and domains in GUI #174

Closed
wants to merge 3 commits into from

Conversation

khalahan
Copy link

This patch allows you to send payments to email address, domain names and url from the bitcoin GUI.
Valid examples of inputs :

Technical explanation

Input url/address is translated to an http request sent to the corresponding domain (ip addresses are not translated for backward compatilibty).
A valid response is a text formatted in json, containing a bitcoin address with an optional label :
{ "error" : "", "label" : "Bitcoin Contact", "address" : "1NMxHnpAE38P9HN9pzRSqAFMCv1WcXZC1N" }
GUI will propose you to add the returned address in your address book.

Discussion on forum : http://bitcointalk.org/index.php?topic=6186.0

@gavinandresen
Copy link
Contributor

Is there a reason you didn't use the HTTP methods in rpc.cpp? bitcoin already knows how to make http requests.

Also, fetching bitcoin addresses via http is insecure-- you're vulnerable to a man-in-the-middle attack. Fetching securely means https and (probably) shipping bitcoin with certificate authority certificates, so you can be sure that you actually are talking to xkcd.com to get their address.

@khalahan
Copy link
Author

I guess methods in rcp wasn't available in the GUI (because they are not used by the GUI, right ?), so I inspired myself by the GetMyExternalIP function (in net.cpp) that retreive remote IP with a GET request. I'll have a look at them (I need a GET request with an uri and Host, I may create one so ?).

Fetching in https would be more secure indeed, but it's not available on every site (and there is a lot of self signed certificates). So, should we check https first and then use http as fallback ?
Bitcoin should use the certifcates of the OS instead of shipping them. Is it possible on windows ?

@TheBlueMatt
Copy link
Contributor

No, falling back on http is a huge security risk (MITM attackers can easily block https).

@khalahan
Copy link
Author

A bitcoin user is vulnerable to a MITM attack because of DNS resolution not using DNSSEC (irc connections, dnsseed and dyndns) and TCP connections to ip not using SSL verifiable certificates. Once he controls all connections...
Possibilities for the attacker are :

  • make the user believe he has been paid by sending a transaction from already spent coins on the real nodes
  • generate fake blocks for confirmations and fake transactions
    So, you can't really trust 100% what is displayed by your bitcoin client ?

By fetching addresses in http, it adds an additional risk :

  • a user can send coins directly to the attacker
    I admit it is a big risk, but it should not block the possibility to fetch an address even if the connection is not secured. However, the user should be warn in this case and decide what to do with a manual action (you won't send 500BTC that way do you ?).

A way to do it securely would be to send the fetch request to connected first level nodes and compare results (or shasum to shorten the message). But, bitcoin network is not ready/designed for trusted nodes.

Another solution is to have a centralized site for address fetching. If a think a centralized solution was good i would already have linked my patch with my domain :p. Maybe, I could fetch both from a direct request to the website and make a second request to one of the trusted ip (declared like bitseed ips) ?

@khalahan
Copy link
Author

  • Use of rpc methods the read the request
  • use of HTTPS only to fetch address
  • using .json instead of .txt

@khalahan
Copy link
Author

Do you have any additional comment on this pull request ?

@gavinandresen
Copy link
Contributor

Did you figure out how to get SSL to verify the identity of the server? Just using SSL isn't sufficient to prevent MITM attacks, SSL has to actually verify that the server you're talking to presents a valid certificate.

@khalahan
Copy link
Author

TheBlueMatt > the port here was not used, but i've cleaned it up, thanks.

gavinandresen > this new patch checks certificate against locales certificates. It is currently configured only for linux by searching in '/etc/ssl/certs'. I don't know if other OS have a common path for certificates or if we should embed them with bitcoin ?

If you want to test :
khal@bitcoin-contact.org : valid CaCert certificate (now :p) => sould return a messageBox with infos in the GUI
khal@sky-animes.com : self signed (and with another domain name) => invalid address

@jgarzik
Copy link
Contributor

jgarzik commented May 6, 2011

The only remaining issue with this change, IMO, is privacy.

This change enables easier external observation of the precise moment when a bitcoin user is making a transaction.

As such, these lookups should be disabled by default, and proactively enabled by the user via command line switch or GUI option checkbox.

@TheBlueMatt
Copy link
Contributor

I don't know what kind of work would be needed, but it would be really nice to get this to work on OSX and/or Win32

@khalahan
Copy link
Author

khalahan commented May 6, 2011

Include a list of root certificates with bitcoin i guess. Is-it an acceptable situation ?

@TheBlueMatt
Copy link
Contributor

That is what many browsers do, but I don't think we want to support that, do we? eg, Mozilla had to ship an update to firefox to blacklist certificates when one CA was compromised recently.

@ByteCoin
Copy link

There's no secure way of doing this. If someone wants to send bitcoins they have to have an address to send to. It's their problem how to verify that the address is correct. The bitcoin client shouldn't try to do this because it's too hard and when it is compromised, it will be blamed. I'm reminded of the old saying "Every application grows until it can send email". How about we concentrate on our core function?

dexX7 added a commit to dexX7/bitcoin that referenced this pull request Aug 20, 2015
0bb8f2a Remove description from pending transaction objects (dexX7)
e6cfb41 Fetch unconfirmed trasanction information for RPC/UI directly (dexX7)
eeea931 Support unconfirmed transactions via omni_gettransaction (dexX7)
glv2 referenced this pull request in glv2/peercoin Mar 21, 2016
deadalnix pushed a commit to deadalnix/bitcoin that referenced this pull request Dec 13, 2016
Add missing cs_orphancache lock to expedited block processing
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Dec 9, 2017
Rebranding of debug window (show command-line options)
classesjack pushed a commit to classesjack/bitcoin that referenced this pull request Jan 2, 2018
attilaaf pushed a commit to attilaaf/bitcoin that referenced this pull request Jan 13, 2020
…#174)

CORE-290 Lock cs_main before calling StreamBlockFromDisk in unit tests.

Approved-by: Arkadiusz Kolodziejski <a.kolodziejski@nchain.com>
Approved-by: Chris Gibson <c.gibson@nchain.com>
Approved-by: Daniel Connolly <d.connolly@nchain.com>
cryptapus added a commit to cryptapus/bitcoin that referenced this pull request Feb 17, 2020
Losangelosgenetics pushed a commit to Losangelosgenetics/bitcoin that referenced this pull request Mar 12, 2020
rajarshimaitra pushed a commit to rajarshimaitra/bitcoin that referenced this pull request Aug 5, 2021
…tcoin#270)

- similar to PR bitcoin#268 and PR bitcoin#269 
- issues bitcoin#174 and bitcoin#175 
- 1 occurrence only 
- "Lightning Invoice" was already used in text, this fixes also the inconsistency
rajarshimaitra pushed a commit to rajarshimaitra/bitcoin that referenced this pull request Aug 5, 2021
)

- see also Issue bitcoin#174 
- see also Issue bitcoin#175 
- is is also "Bitcoin Node", not "Bitcoin Network Node"
- simpler is better, shorter is better
- 7 occurrences were replaced
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants