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

TLS: Client authentication with client certificate CN #716

Closed
wants to merge 1 commit into from

Conversation

ph4r05
Copy link
Contributor

@ph4r05 ph4r05 commented Dec 3, 2015

TLSOPS module was extended to add support for checking correspondence between FROM/TO URIs and CN of the client certificate used for TLS connection = client authentication via client certificates.

For clients using TLS client certificates this patch can save bandwidth and messages up to 50% for REGISTER, MESSAGE and INVITE requests compared to traditional www_authorize authentication. This improvement is especially important for clients connected via mobile networks (higher packet loss / latency).

Example configuration for TLS client authentication:

# authenticate the REGISTER requests 
if (proto==TLS && is_peer_verified()){
    xlog("L_NOTICE","[$pr:$fU@$si:$sp]: Processing '$rm' auth trusted cert '$tls_peer_subject_cn'\n");

    # Doing pretty serious stuff here, check if to matches CN.
    if (!tls_check_to())
    {
        xlog("L_NOTICE","[$pr:$fU@$si:$sp]: Processing '$rm' auth TO check failed\n");
        sl_send_reply("403","Forbidden auth ID");
        exit;
    }
}
else {
    # TLS validation could not be applied - use challenge response
    $var(auth_code) = www_authorize("", "subscriber");
    xlog("L_NOTICE","[$pr:$fU@$si:$sp]: Processing '$rm' auth: '$var(auth_code)'\n");

    if ( $var(auth_code) == -1 || $var(auth_code) == -2 ) {
        xlog("L_NOTICE","Auth error for $fU@$fd from $si cause $var(auth_code)");
    }
    if ( $var(auth_code) < 0 ) {
        www_challenge("", "0");
        exit;
    }
    if (!db_check_to())
    {
        sl_send_reply("403","Forbidden auth ID");
        exit;
    }
}

@razvancrainea razvancrainea self-assigned this Jan 19, 2016
@razvancrainea
Copy link
Member

The patch looks ok, but I think we can do this a little bit more flexible: instead of having two functions tls_check_from and tls_check_to, why don't we add a single one, i.e. tls_check_username("user"), that can receive a pvar as input, and checks the certificate username against it.

This way you can practically authenticate the client based on its alias, or user-account, instead of simple to or from usernames.

What do you think about this approach?

@ph4r05
Copy link
Contributor Author

ph4r05 commented Feb 23, 2016

Thanks for checking this! Yes, you are right, it would be better. I will reimplement it like you suggested and update the pull request. OK?

@razvancrainea
Copy link
Member

Definitely, thanks!

@ph4r05
Copy link
Contributor Author

ph4r05 commented May 22, 2016

Hi! I was checking the way how to re-implement it according to your suggestion.

There actually is a very universal way for comparing certificate CN with something else. The module exports pvar tls_peer_subject_cn which can be used for comparison in script as you suggested, right?

With this pull request I wanted to add an easy way to get to message fields To and From, take just username@hostname from those URIs and compare them to the CN in the peer certificate. I considered this as the most common use-case when comparing CN.

I guess tls_check_from() it is equivalent to:

$tls_peer_subject_cn == ($(fu{uri.user}) + "@" + $(fu{uri.host}))

I was wondering whether to implement a new check function which would accept pvar, but as I mentioned above it may be redundant.

What would you suggest to implement? Do you have an idea how the exported function should look like and what type of input it should take?

Thanks a lot for suggestion!

@razvancrainea
Copy link
Member

You could do a new function tls_check_user("format") that accepts a format (i.e. something like $fU@$fd), but I don't see too much value of it. The way it is done now is more flexible and I think easier.

If you do decide to implement the function, just open a new PR and I'll take a look.

Best regards,
Răzvan

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.

None yet

2 participants