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

Update Regex for QRcode scanned VCard #2600

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Awbmilne
Copy link

@Awbmilne Awbmilne commented Oct 15, 2020

Fix issues with QRCode VCard Key importing | closes #2599 closes #2601

Regex needs to be corrected for handling values in a VCard format.

It also seems possible to extend the import functionality to the URL syntax of the Vcard Standard.

Description

Currently:

  • Fix parsing of KEY:OPENPGP4FPR:[HASH] with better regex
  • Extend regex to full-length search URLs in VCard standard format
    For example: KEY;MEDIATYPE=application/pgp-keys:https://keyserver.ubuntu.com/pks/lookup?op=get&search=0x[40-CHAR-HASH]

Prospective:

  • Extend to All VCard formats using URL import
    2.1:   KEY;PGP:http://example.com/key.pgp
    3.0:   KEY;TYPE=PGP:http://example.com/key.pgp
    4.0:   4.0: KEY;MEDIATYPE=application/pgp-keys:http://example.com/key.pgp
    

Motivation and Context

I have been looking to streamline my VCard Contact and PGP information, This would make it easier to deal with on android. Easier to share Keys and Contact info with same QRcode on Business card or such. Refer to #2599

How Has This Been Tested?

Hasnt been tested, Dont have the Facilities to do so... Sorry.
Its just regex though... So it should work fine.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)
  • ✅ New feature (non-breaking change which adds functionality)

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2020

CLA assistant check
All committers have signed the CLA.

@Awbmilne
Copy link
Author

For someone more experienced with this project...

Is there a significant reason we cannot import PGP keys from VCards that have the VCard standard syntax?
Why we cant import from custom URL?

This seems like a really useful feature... But maybe there is a security reason or something else?

It seems to me that looking up PGP keys by Fingerprint on the PGP servers would be the same security as from a custom https:// host. The VCard maker would have to have set that URL, same as setting the Fingerprint in the custom VCard field.

- URL must contain full 40 character Fingerprint
- Handles most SKS Server keys
- Handles keys.openpgp.org
- Handles OPENPGP4FPR
@Awbmilne
Copy link
Author

Awbmilne commented Oct 15, 2020

With the above commit a1fed15:

 ─── Regex Explanation ───
 - Full String -
(?<=\n)(?:KEY:OPENPGP4FPR:|KEY.*?https.*?\/pks\/lookup\?(?:op=get&)?search=0x|KEY.*?https:\/\/keys.openpgp.org\/vks\/v1\/by-fingerprint\/)([a-fA-F0-9]{40})(?=\n)

?:KEY:OPENPGP4FPR:                                           <- OPENPGP4FPR values
KEY.*?https.*?\/pks\/lookup\?(?:op=get&)?search=0x           <- Majority of SKS Server pool URLs
KEY.*?https:\/\/keys.openpgp.org\/vks\/v1\/by-fingerprint\/  <- keys.openpgp.org URLs
([a-fA-F0-9]{40})                                            <- Fingerprint

Matches:
KEY:OPENPGP4FPR:[HASH]
(Most) PGP Server URLs with Full Hash embedded

Examples:
KEY:OPENPGP4FPR:ABAF11C65A2970B130ABE3C479BE3E4300411886
KEY;PGP:https://keyserver.ubuntu.com/pks/lookup?op=get&search=0xABAF11C65A2970B130ABE3C479BE3E4300411886
KEY;TYPE=PGP:https://keys.openpgp.org/vks/v1/by-fingerprint/ABAF11C65A2970B130ABE3C479BE3E4300411886
KEY;MEDIATYPE=application/pgp-keys:https://pgp.mit.edu/pks/lookup?search=0xABAF11C65A2970B130ABE3C479BE3E4300411886
KEY;MEDIATYPE=application/pgp-keys:https://keyserver.ubuntu.com/pks/lookup?op=get&search=0xabaf11c65a2970b130abe3c479be3e4300411886

Edit: Update to reflect regex improvements in later commits too (a1fed15...fc3dd86)

@Awbmilne Awbmilne marked this pull request as ready for review October 15, 2020 04:07
@wiktor-k
Copy link
Contributor

wiktor-k commented Oct 16, 2020

Hi @Awbmilne, the reason why fingerprint format is supported but not arbitrary URLs is the following: the fingerprint provides end-to-end verification of the key while the URL could point to any key.

HTTPS does not protect against malicious server/VPS admin replacing the key or against your domain expiring and someone squatting your domain name. The fingerprint format on the other hand does not relay on the format but verifies that the key is the correct one.

Do note that business cards don't change often so it's important to have a correct key info all the time there.

Edit:

It seems to me that looking up PGP keys by Fingerprint on the PGP servers would be the same security as from a custom https:// host. The VCard maker would have to have set that URL, same as setting the Fingerprint in the custom VCard field.

This would be fine too 👍

Edit 2:

It seems the issue with your QR encoded VCard in the original issue is that you used \r\n instead of \n to separate VCard fields:

curl -sSL https://user-images.githubusercontent.com/50780746/96058806-4147c480-0e5a-11eb-94a3-7d2ec8125888.jpg | zbarimg -q - | xxd

(see 0d0a there?) If you fix that then it'd work. The regexp of course could be adjusted and that's a good thing but I'm personally missing unit tests here as the regexp become more complex :)

@Awbmilne
Copy link
Author

Thanks @wiktor-k

issue with your QR encoded VCard in the original issue is that you used \r\n instead of \n

Good find! Wouldve never though of line endings. Windows is really a pain for raw text files.🥴
I made changes to my suggested Regex in 87d4bda, so that it can handle CRLF and LF line endings.

HTTPS does not protect against malicious server/VPS admin replacing the key or against your domain expiring and someone squatting your domain name. The fingerprint format on the other hand does not relay on the format but verifies that the key is the correct one.

This is a good point, I agree. Forcing the use of well-established and well-maintained key servers is a good idea.

Luckily, My existing changes comply with this sentiment!
The purpose of the Regex is to extract the 40 Character Key Fingerprint from the KEY: field's URL. OpenKeychain then uses that fingerprint to search for the key on the established Key Servers. This means the URL is never used, it just serves as another format for the Fingerprint.
While this doesnt work for direct download links of Public Key Files, Like: It does work for Links to Public Keys pages on SKS Servers. For example, my own key: http://keys.gnupg.net/pks/lookup?op=get&search=0x6E8329BB07B245820C91E88B409B6A5A30BE7AAD

This may not be useful for many people, but it does expand the options for QRcode based VCards.

If you are looking for unit test examples, Lines 154-158 has some typical examples of URLs that this type of Regex fingerprint extraction should work for. (Fingerprint used being Linus Torvalds' Key. May want to change for unit tests, as his key is multiple MB these days)

@Awbmilne
Copy link
Author

Awbmilne commented Oct 18, 2020

IMPORTANT NOTE:

The VCard standard RFC6350 states in Section 3.2 that:

Individual lines within vCard are delimited by the [RFC5322] line break, which is a CRLF sequence (U+000D followed by U+000A).

Meaning that VCard files are "supposed" to be standardized with CRLF (\r\n) line ending.

Personally, This seems like a dumb decision for the standardization. (Feels like Microsoft had some financial pull there)
But, I believe it would be best to handle both Unix and Windows line ending.

The commits above handle both cases (though, Unit testing should be added for verification, Which is past my knowledge).

This issue has been documented in #2601

@Awbmilne
Copy link
Author

Secondary Note:

The openpgp4fpr:<fingerprint> standard outlines that the Fingerprint is:

... exactly 40 characters of lower and upper-case letters A, B, C, D, E, F and digits (0-9).

The use of Uppercase and Lowercase digits is accounted for in the new Regex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants