Add a URI decoder, with tests by levitte · Pull Request #1961 · openssl/openssl

levitte · 2016-11-19T10:27:10Z

Checklist

documentation is added or updated
tests are added or updated
CLA is signed

Description of change

The test uses test/uritests.txt. That file could use some more tests, please suggest some.

Fixes #1957

richsalz · 2016-11-20T04:07:48Z

have you looked at any tests from w3c? Google it, there's a bunch.

levitte · 2016-11-20T06:19:16Z

Most tests are about uri joining, which this module doesn't do.
I finally found a file with a large number of "interesting" uris, though. I'm currently working through it to fill uritests.txt

levitte · 2016-11-20T17:11:02Z

Ok, that's a load of test URIs, and the bugs they helped clear out fixed

levitte · 2016-12-06T00:54:14Z

Rebased on top of a fresher master

levitte · 2017-02-21T12:35:53Z

Update: there is now a function to decode the authority into its smaller parts, user, password, host and service, as well as a function to do percent decoding inline.

richsalz

you find it useful/interesting to look at the stanza stuff in bntest.c

richsalz · 2017-03-01T21:08:48Z

test/uri_internal_test.c

+}
+
+static int linecounter = 0;
+static TEST_DATA *read_test_data(FILE *f)


this is a useful function; e.g., bntest and exptest could use it.

richsalz · 2017-03-01T21:09:15Z

test/uri_internal_test.c

+
+        if (*p == '\n') {
+            if (test_data != NULL)
+                break;


add skip lines starting with #

richsalz · 2017-03-01T21:17:45Z

Actually, perhaps go the other say. Look at test/bntest.c, which has a generic key/value reader. Pull that out and then use it in your code here, with semantics on top of the PAIRs.

levitte · 2017-03-01T21:42:29Z

Ok, @mattcaswell, I think I've responded to all your comments.

levitte · 2017-03-01T21:44:18Z

The better idea would be to make a stanza library for everyone to use instead of doing reuse-by-copy.

richsalz · 2017-03-01T21:45:09Z

well, yeah, that's what i was suggesting you do :)

levitte · 2017-03-01T21:48:44Z

Ah. Frankly, I'd rather do that refactor in a separate PR.

levitte · 2017-03-01T22:09:40Z

@richsalz, I noticed your comments about uri_charmap, even though github seems to have lost them.

mattcaswell

I took another scan through this and picked up a few more nits.

The main stumbling block for me on this is the licence question.

mattcaswell · 2017-03-01T23:49:47Z

crypto/uri.c

+                            if (++dblcolon > 1)
+                                break;
+                            maxcolons -= 2;
+                        } else


mattcaswell · 2017-03-01T23:50:14Z

crypto/uri.c

+
+    /* Check if the loop stopped because of an invalid character */
+    if (!is_valid(p[off])) {
+        char tmpbuf[200];


newline after this

mattcaswell · 2017-03-01T23:52:36Z

crypto/uri.c

+{
+    const char *p = uri;
+
+    OPENSSL_init_crypto(0, NULL);


I'm wondering if this is actually necessary?

It's been some time, I do recall there was a reason but cannot remember what it was...

It would be good to know. The thing is we are supposed to auto-initialise if we call any functions that need it. These functions don't seem to need it themselves (e.g. no shared global data etc), so if this call was necessary then it suggests some other function call was made that didn't auto-initialise when (possibly) it should have done.

It might be something in the STORE effort that got jinxed... Anyway, I've just taken those inits away, things are still working fine, so...

mattcaswell · 2017-03-01T23:57:09Z

crypto/uri.c

+{
+    const char *p = authority;
+
+    OPENSSL_init_crypto(0, NULL);


mattcaswell · 2017-03-01T23:57:42Z

test/uri_internal_test.c

+        } else if (strcasecmp(key, "scheme") == 0
+                 || strcasecmp(key, "decoded_scheme") == 0) {
+            if (test_data != NULL && test_data->expected.scheme == NULL) {
+                test_data->expected.scheme = value;


Weren't you going to do this stuff in a macro?

levitte · 2017-03-02T06:32:09Z

As for licensing, if we don't get an answer, I'll simply take it all away and we'll just add new cases as time goes.

…o calls

levitte · 2017-03-02T14:06:04Z

I just discovered that the IPv6 address parser failed on some perfectly acceptable addresses...

…om a document under CC0

levitte · 2017-03-02T15:30:48Z

I rewrote test/uritests.txt to only include URIs I've written myself, or some taken from a CC0 licensed document (verifiable)

Also fixed a few errors in the IPv6address check

levitte · 2017-03-02T21:11:17Z

With a simplification of #2011, this turned out not to be necessary any more.

@cweb, sorry about the noise.

dwmw2 · 2017-06-01T10:45:13Z

This is abandoned, right? If it comes back, does it need to support RFC7512 URIs too?

I was actually inclined to just use libp11-kit for a PKCS#11 STORE so I don't really need it, but if OpenSSL has a URI parser then it would make sense for me to use it.

levitte · 2017-06-01T10:51:06Z

It is abandoned, yes.

levitte added the branch: master Applies to master branch label Nov 19, 2016

levitte added this to the 1.1.1 milestone Nov 19, 2016

levitte self-assigned this Nov 19, 2016

levitte mentioned this pull request Nov 19, 2016

WIP: Add the STORE module, for loaded certs and keys (and more?) given a URI #1962

Closed

3 tasks

levitte force-pushed the uri branch from c1876af to 01889ee Compare November 19, 2016 11:22

levitte mentioned this pull request Nov 28, 2016

Add the STORE module take 2, for loaded certs, keys and more given a URI #2011

Closed

3 tasks

levitte force-pushed the uri branch 2 times, most recently from f5420c4 to 9a51a9f Compare December 6, 2016 00:52

levitte force-pushed the uri branch 4 times, most recently from ae6e7d0 to 927f8fe Compare December 14, 2016 11:13

levitte force-pushed the uri branch from 927f8fe to 9c81580 Compare January 11, 2017 18:06

levitte force-pushed the uri branch from 9c81580 to 5104b1e Compare January 25, 2017 18:19

levitte force-pushed the uri branch from 5104b1e to afdca54 Compare February 7, 2017 17:05

levitte force-pushed the uri branch 3 times, most recently from fb1c130 to 034c36e Compare February 20, 2017 01:57

levitte mentioned this pull request Feb 20, 2017

STORE search capability #2688

Closed

2 tasks

levitte force-pushed the uri branch 2 times, most recently from e17b89d to d4212d4 Compare February 21, 2017 12:29

This was referenced Feb 21, 2017

Rework crypto/x509's by_file.c and by_dir.c to work with STORE, take 1 (the light take) #2696

Closed

Rework crypto/x509's x509_lu.c to work with STORE, take 2 (the radical take) #2697

Closed

levitte force-pushed the uri branch 2 times, most recently from aa3b35d to e18c0c7 Compare February 22, 2017 20:54

levitte mentioned this pull request Mar 1, 2017

License question cweb/iri-tests#1

Open

richsalz reviewed Mar 1, 2017

View reviewed changes

levitte added 2 commits March 1, 2017 22:41

[to be squashed] fixups of crypto/uri.c

7a9bcb3

[to be squashed] fixups of test/uri_internal_test.c

e2ce0f7

levitte added 3 commits March 1, 2017 23:10

[to be squashed] fixups of test/uri_internal_test.c

9babefd

[to be squashed] fixups of crypto/uri_charmap.*

d0acacd

[to be squashed] fixups of test/uri_internal_test.c

0fa1b5c

mattcaswell requested changes Mar 1, 2017

View reviewed changes

[to be squashed] fixups of crypto/uri.c

eba601c

levitte force-pushed the uri branch from a656d12 to c17e1d4 Compare March 2, 2017 05:36

[to be squashed] fixups of test/uri_internal_test.c

b22386e

levitte force-pushed the uri branch from c17e1d4 to b22386e Compare March 2, 2017 05:36

[to be squashed] fixups of crypto/uri.c, take away OPENSSL_init_crypt…

489f5cb

…o calls

levitte added 2 commits March 2, 2017 16:28

[to be squashed] fixups of crypto/uri.c, fix IPv6 parsing

d7d5a6b

[to be squashed] New set of test cases, invented in house or taken fr…

601d61f

…om a document under CC0

levitte requested review from mattcaswell and richsalz March 2, 2017 15:31

levitte closed this Mar 2, 2017

Uh oh!

Conversation

levitte commented Nov 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

richsalz commented Nov 20, 2016

Uh oh!

levitte commented Nov 20, 2016

Uh oh!

levitte commented Nov 20, 2016

Uh oh!

levitte commented Dec 6, 2016

Uh oh!

levitte commented Feb 21, 2017

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Mar 1, 2017

Uh oh!

levitte commented Mar 1, 2017

Uh oh!

levitte commented Mar 1, 2017

Uh oh!

richsalz commented Mar 1, 2017

Uh oh!

levitte commented Mar 1, 2017

Uh oh!

levitte commented Mar 1, 2017

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Mar 2, 2017

Uh oh!

levitte commented Mar 2, 2017

Uh oh!

levitte commented Mar 2, 2017

Uh oh!

levitte commented Mar 2, 2017

Uh oh!

dwmw2 commented Jun 1, 2017

Uh oh!

levitte commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

levitte commented Nov 19, 2016 •

edited

Loading