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

Venafi doesn't escape challenge locations in JSON data. #40

Closed
fieldju opened this issue Nov 2, 2017 · 4 comments
Closed

Venafi doesn't escape challenge locations in JSON data. #40

fieldju opened this issue Nov 2, 2017 · 4 comments

Comments

@fieldju
Copy link

fieldju commented Nov 2, 2017

Venafi does not escape the uri's in the json data

ex
https://domain.venafi.com/vacme/v1/authz/{11111111-222222-333333-44444-555555}/0

In org.shredzone.acme4j.challenge.Challenge::getLocation()

/**
 * Returns the location {@link URI} of the challenge.
 */
@Override
public URI getLocation() {
    return data.get(KEY_URI).asURI();
}

a java.net.URISyntaxException gets thrown because of "Illegal character in path at index ..."

I can work around it locally via Guava and modifying asURI in the org.shredzone.acme4j.toolbox.JSON::asURI()

import com.google.common.net.UrlEscapers;

...
...

/**
 * Returns the value as {@link URI}.
 *
 * @return {@link URI}, or {@code null} if the value was not set.
 */
public URI asURI() {
    if (val == null) {
        return null;
    }

    try {
        return new URI(UrlEscapers.urlFragmentEscaper().escape(val.toString()));
    } catch (URISyntaxException ex) {
        throw new AcmeProtocolException(path + ": bad URI " + val, ex);
    }
}

Im not sure if this is an issue with this client or the Venafi ACME V1 IMPL, but their impl does work with certbot.

@shred
Copy link
Owner

shred commented Nov 2, 2017

Actually, in the ACME v2 branch, commit f38002c changes all resource URI to URL. I guess I need to backport the commit to the master branch, to fix this issue.

@fieldju
Copy link
Author

fieldju commented Nov 2, 2017

That would be nice, also I think I did find out that they have V2 support planned, but I have no date.

I'm putting my Venafi work on pause right now too, because there ACME impl only supports HTTP-01 challenges which doesn't work for my use case, I have to wait till they support DNS-01.

@shred
Copy link
Owner

shred commented Nov 2, 2017

It's good you're on pause. The backport is more difficult than I had expected first, so it is going to take a little while anyway. 😉

Thank you for your input! I didn't knew there is another ACME server implementation besides Let's Encrypt. If Venafi support requires more changes, please open a new issue or PR.

shred added a commit that referenced this issue Nov 4, 2017
@shred
Copy link
Owner

shred commented Nov 4, 2017

It should be working in the master branch, but I cannot test it myself... I'm closing this issue for now.

@shred shred closed this as completed Nov 4, 2017
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

No branches or pull requests

2 participants