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

HTTPS transport, revised #86

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@nalind
Copy link
Contributor

nalind commented Mar 20, 2014

This is a revised version of pull request #31, adding a couple of interoperability fixes, use of certificate anchors and hostname verification, self-tests, and some documentation.

It's been tested against a Windows Server 2012 server and the python-based server from https://github.com/npmccallum/kdcproxy with patches from its pull requests 5, 6, and 7 added.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 20, 2014

Just from looking at the commit subjects:

Please make separate pull requests for "Don't check the kpasswd server's reply address", "Check for strtoul() correctly," and any other commits which fix bugs independent of HTTP proxy support.

"Pull request comments, #1" should not be in the commit series; it should be folded into the commits it applies to. Similarly for "Fix overflow in the HTTPS reply receipt buffer" and any other commits which address problems with previous commits in the pull request.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Mar 20, 2014

Will do.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Mar 21, 2014

Patch set updated. I think the test script could be clearer in its choice of variable names, and I may push an update to it, but I don't have anything else pending.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 24, 2014

Another note after quickly scanning through: the ASN.1 codecs are missing test cases. In lib/krb5/asn.1/README, see the section on "Writing test cases".

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Mar 24, 2014

Okay, I've updated the codec and HTTPS patches to add tests for the codecs and move the handling of the extra length bytes into the ASN.1 library so that the codec tests would exercise that as well.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 26, 2014

First commit (transport enumerator):

  • t_locate_kdc doesn't compile without later commits because it uses the HTTP value.
  • The commit message contains a >80-character line.
  • "portable" seems like the wrong word in the commit summary. I would just remove the word.

Second commit (build system support):

  • The commit message is too brief.
  • Some of the lines added to configure.in are more than 79 characters long.
  • The default is "no" but the help string says the default is openssl.
  • This uses pkg-config to find OpenSSL; other uses of OpenSSL do not. It seems like we should be consistent.
  • The name "https-crypto-impl" is not really descriptive, as we are looking for a TLS library, not a crypto library. We could go with "proxy-tls-impl", perhaps.
  • HTTPS_CRYPTO_IMPL_CFLAGS is added to LOCALINCLUDES in lib/krb5/Makefile.in, but OpenSSL is used in sendto_kdc.c in lib/krb5/os. Ideally it should be applied only to the file it's used in, but I'm not sure how difficult that is with our build system.
  • HTTPS_CRYPTO_IMPL is defined for Makefiles but never used. (Perhaps it should be, with OpenSSL invocations made from functions in a separate file from sendto_kdc.c, but I haven't looked at the later patches enough to judge that.)
  • HTTPS_CRYPTO_IMPL_CFLAGS and HTTPS_CRYPTO_IMPL_LIBS should be expanded into variables and used with $(varname) so they can be overridden at build time, not directly used as @varname@.

Commit 3 (remaining cm read/write mutator functions):

  • The commit message is too brief, and the summary makes no sense out of context.
  • This patch creates unused function warnings, but I think that's okay.
  • After this patch, three of the cm_ functions use preprocessor markup inside the functions and the other four are defined separately for poll and select. We should be consistent, probably defining all seven functions separately.
  • There should be a static inline helper to find the poll entry by fd, returning a pointer to the poll entry, instead of six different repetitions of the for loop.
  • Looking at how these four functions are used, I think they are the wrong internal abstraction. I will prepare a commit which is independent of proxy support and puts the code into a better starting point.

I haven't reviewed commits 4-9 yet.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 26, 2014

Please review #95 which should be able to replace the third commit. It will require some adjustments to the later commits to use the revised cm_add_fd/cm_read/cm_write contracts.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Mar 26, 2014

Looks fine to me. Will switch to it and work on the rest.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 27, 2014

Feedback on patch 4 (codecs):

  • The type name krb5_kkdcp_proxymessage is a little weird, since the "p" in MS-KDDCP stands for "proxy," and the Microsoft document names the sequence KDC-PROXY-MESSAGE. I would accept krb5_kdc_proxy_message or krb5_kkdcp_message.
  • I personally would have used shorter names for the first two fields (probably "message" and "realm"), but I can understand wanting to hew to the ASN.1 field names. Matter of preference.
  • I don't think asn1_k_encode.c should be responsible for adding and removing the four-byte length. How problematic would it be to make the calling code include the length when constructing krb5_kkdcp_proxymessage structure and expect the length when consuming one?
@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Mar 27, 2014

Okay, I'll change the structure name. Earlier versions handled the length bytes in sendto_kdc.c, I can move that logic back, too.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Mar 27, 2014

Okay, I think this addresses the things mentioned so far.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 28, 2014

Feedback on the "Dispatch-style protocol switching" commit:

  • The add_connection changes do not seem to be related to dispatch-style protocol switching, except for the lines to set function pointers. I think those changes belong in the "Use k5_transport" commit. The corresponding changes to resolve_server as well.
  • Renaming set_conn_state_msg seems to belong in a later commit, when the meaning of the function actually changes; it has nothing to do with dispatch-style transport functions.
  • The expansion of the comment at line 653 is unnecessary in my opinion.
  • It is not our practice to try to suppress compiler warnings for unused parameters, so the "(void)ssflags;" line in service_kill_conn is not required. (This comment may be mooted by the next bullet point.)
  • There is no need to set handlers for connect and write for the UDP transport, and therefore to force service_kill_conn to match the fd_handler_fn prototype. UDP connections are always in read state. service_dispatch can use asserts to check that handlers are non-null before calling them, to get better stack traces if we're worried about bugs of that nature.
  • The new code structure makes it the responsibility of an fd_handler_fn to sanity-check ssflags against the connection state, which is unneccesary. service_dispatch should check for exceptions and kill the connection, and service_dispatch can use asserts if we're worried about bugs where we were listening for reading when we were in write state or vice versa. fd_handler_fn does not need an ssflags parameter. (Also, this is moot, but the check at the beginning of service_tcp_write considers the absurd possibility that it was called with conn->state == READING, and the check at the beginning of service_udp_read considers a similarly absurd possibility.)
  • In service_tcp_read, a better approach to dealing with the long SOCKET_READ line is to make a local variable named "in" which aliases &conn->x.in.
@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 29, 2014

Feedback on "HTTPS transport":

  • There is a typo in the krb5_conf.rst addition, "cetificate" instead of "certificate".
  • Add the realm to the k5_sendto function signature. Don't copy the realm into every server_entry. (When this is fixed, there shouldn't be any changes to add_addr_to_list since modules can't produce HTTPS entries, and add_host_to_list should be dramatically simpler since it only has one argument to copy.)
  • The identifier "suburi" suggest a subsidiary URI to me, not a piece of a URI (and if it did suggest part of a URI, it would be vague about which part). Please rename that to "uri_path" wherever it appears.
  • (Aside: the socktype argment of k5_locate_kdc doesn't interact very intuitively with HTTPS. It would be better to replace it with a "no_udp" flag, in a separate commit. I won't make that a requirement for this pull request.)
  • The copyright statement in sendto_kdc.c needs to come with a license statement, I think. I can discuss that with Tom.
  • make_proxy_request does not need to discuss the failings of earlier versions of the [MS-KKDCP] document. The first sentence of the comment should be sufficient.
  • make_proxy_request should use store_32_be, not htonl and memcpy.
  • make_proxy_request and its caller do not do proper error handling. (Also, we do not "return;" at the end of functions returning void, but make_proxy_request should not return void.)
  • make_proxy_request accesses and modifies its output parameters after setting them. This is not our current practice. Add local variables if necessary, but only touch the output parameters to set them to their final values once those are known. (A null check after allocating is sometimes an acceptable exception, but I don't think that applies in this case.)
  • set_conn_state_msg assigns allocated memory to x.out.sgbuf[0]. As far as I can tell, this memory is never freed. It would not be appropriate for sgbuf[0] to be conditionally freed (our practice does not allow for pointer fields which sometimes own memory and sometimes alias it), so that memory needs to be remembered in a new, separate field of the conn_state and freed
  • (Aside: the "x" element of struct conn_state is completely vestigial now that r14742 made it a structure instead of a union. We should get rid of it at some point. Not a requirement for this pull request.)
  • SSL_library_init and SSL_load_error_strings should be done from the library initialization function, not every time we connect. Perhaps that belongs in the build support commit.

I'm going to pause at this point and note that, with the current state of this patch and upstream OpenSSL, I expect to see serious practical issues with thread-safety, because we do not register locking callbacks. Registering locking callbacks is probably not the answer (it could cause other users of OpenSSL to crash if libkrb5 is unloaded, for instance), but I'm not sure we can ignore the problem either.

I expect to have more feedback on this commit, since I haven't looked closely at the read and write functions. But this is getting long.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Mar 31, 2014

More feedback on "HTTPS transport":

  • The commit message needs elaboration.
  • The idiom seems to be SSL_CTX_new(SSLv23_client_method()). SSLv23_client_method cannot fail.
  • Is it expensive to create an SSL_CTX object? If so, we might want to keep it around in the krb5_context (probably in the os_context). If not, SSL_new adds a reference count to the SSL_CTX, so there is no need to keep it around. http.ctx can be removed, we can free the SSL_CTX object right after we pass it to SSL_new, and SSL_free will get rid of it.
  • Perhaps we should check whether the connection succeeded before doing SSL stuff?
  • The direct call to krb5int_trace in service_ssl_connect is not okay.
  • service_ssl_connect does not appear to call service_ssl_write like service_tcp_connect does.
  • SSL_write does not mention SSL_WANT_CONNECT as one of the things that can happen, so I don't think we should try to handle it in service_ssl_write. It doesn't look like we're doing anything correct with that anyway. Likewise in service_ssl_read.
  • We haven't set SSL_MODE_ENABLE_PARTIAL_WRITE, so SSL_write will only succeed (nwritten > 0) if it has written the complete message. I haven't dived into the OpenSSL library deeply enough to make sure it does the write thing on repeated calls when it needs to do SSL stuff after writing part of the message, but assuming it does, we don't need any of the SG_ADVANCE stuff; we can just assume that it's time to switch to reading the response.
  • service_ssl_write has too much copy-paste from service_tcp_write. The first part can probably go away because of the previous bullet point. Most of the second part (zeroing all of the conn->x.in fields) isn't needed since conn is allocated with calloc, and can actually be removed from service_tcp_write in the "Dispatch-style protocol" commit.
  • The naming of service_{ssl,https}_{read,write} is confusing. service_ssl_read appears to be a helper function, not a dispatch function. All three HTTP dispatch functions should use the same prefix, and service_ssl_read should be named differently.
  • In service_ssl_read, the length argument to SSL_read seems wrong. It subtracts out x.in.bufsizebytes_read, which is never incremented in the SSL case, and doesn't subtract out the number of bytes previously read. Best would probably be to start using x.in.n_left like service_tcp_read does. (Set it to bufsize on the first read, and reduce it when we increment x.in.pos.)
  • I'm not sure that allocating a flat megabyte for reading is a great idea. I think we can start at 8K and double as needed up to 1MB (whenever there's less than 1K in x.in.n_left, say) without adding too much code to the initialization block.
  • service_https_read uses a krb5_data in a really non-intuitive way. Use separate local variables until you're ready to call decode_krb5_kkdcp_message, then use make_data() to assign buf.
  • Using strtoul when reading the content length would avoid having to think about integer overflow, even if it doesn't result in fewer lines of code.
  • The length sanity check doesn't accomplish much since it doesn't take the header size into account.
  • The big comment after decode_krb5_kkdcp_message should be pared to its first sentence. The ensuing code should use load_32_be, but should also check that it didn't get a <4-byte response.
  • Check all added comments for punctuation and capitalization; some of them don't have it.
@tsitkov

This comment has been minimized.

Copy link
Contributor

tsitkov commented Mar 31, 2014

Is SSLv2 still relevant? Should we use SSLv3_client_method() instead of SSLv23_client_method()?

@tsitkov tsitkov closed this Mar 31, 2014

@greghudson greghudson reopened this Mar 31, 2014

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Apr 2, 2014

If I'm reading the docs right, using SSLv3_client_method() limits the client to using SSLv3, and the recommended procedure for turning off SSLv2 is to use SSLv23_client_method(), and then set the SSL_OP_NO_SSLv2 option. I'll make that change.

I don't have a good feel for the relative cost of SSL_CTX and SSL structures, but it sure looks like there's work to be saved by storing the CTX somewhere other than the per-connection state. I'm not sure about the os_context being the right place, so I didn't go in and do that yet, but I'm not opposed to doing that if we don't want to add something specifically for the lifetime of the k5_sendto() call.

I went ahead and started setting SSL_MODE_ENABLE_PARTIAL_WRITE, since it looks like we already have to handle toggling between waiting for the socket to become readable and writable during both the READING and WRITING states. A quick spot-check afterward didn't turn up any problems, but if there are any, I figure we're going to want to find them sooner rather than later to avoid blocking.

I think the other notes are covered now, but it's quite possible I've missed one or two.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Apr 2, 2014

Are we sure it's necessary to turn on partial-write mode? If OpenSSL will handle it for us under the covers, that's less complexity in our code. The documentation suggests that it will as long as subsequent calls have the same arguments, but we'd want to check that there's logic to handle the case where a renegotiation happens in the middle of a write.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Apr 2, 2014

Do you have specific reservations about caching the SSL_CTX for the lifetime of the context? There are often multiple k5_sendto operations in close sucession, as in a preauthenticated AS request.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Apr 2, 2014

I only have vague reservations about short-lived CRLs being combined with longer-lived contexts, but can accept that we don't expect that to be a common case. Should the SSL_CTX be lazily initialized and just hang around until the os_context is freed, or is there a point where it should be cleaned up before that?

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Apr 2, 2014

I don't have a strong opinion on the partial write mode flag, so we can remove it, as it cuts about half of the write function out. I'm not clear on how renegotiation affects this.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Apr 2, 2014

It looks like OpenSSL 1.1's going to introduce an X509_check_host() function, as there's been discussion on openssl-dev about its implementation this week. Depending on 1.1 is probably not an option, not in the near-term at least, but since it was news to me I figured I should mention it in the context of the verify_servername() function that the anchor loading patch also adds.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Apr 2, 2014

It looks likely that OpenSSL properly handles resumption of non-blocking writes if you call it with the same arguments, so let's allow it to do its job and leave the partial-write flag off.

For X509_check_host, the ideal thing would be:

  1. Make sure our code matches X509_check_host in semantics (at least for ASCII hostnames).
  2. Detect X509_check_host at configure time and use it if it is defined, in preference to our code.
  3. In the future (5+ years from now) we can get rid of our code and require OpenSSL 1.1.

You're sure that using a fresh SSL_CTX causes a CRL refresh and re-using an old one does not? If so, let's create a new context every time for now; it doesn't look super-expensive to me. However, please see my earlier note about not needing to keep the SSL_CTX around in struct conn_state.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Apr 2, 2014

Another round on "Dispatch-style protocol switching":

  • kill_conn is still renamed to service_kill_conn and moved around within the file, even though it is no longer used as a dispatch function.
  • There are still changes in this commit which have nothing to do with dispatch-style flow control: (1) set_conn_state_msg_length reorders UDP vs. TCP; (2) add_connection reorders UDP vs. TCP in addition to setting the new dispatch functions; (3) start_connection has a comment clarification and a removed blank line. The comment clarification does not belong in this pull request series. For UDP vs. TCP, you can leave it out or you can make it a separate commit, but it shouldn't be combined with this one.
  • The decision to reorder code so that UDP appears first means that there is a mix of unrelated changes in this commit. The changes to set_conn_state_msg_length have nothing to do with dispatch-style flow control, the changes to add_connection are a mix of reordering and behavior changes.
  • In service_dispatch, use asserts instead of if-aborts for conciseness.
@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Apr 2, 2014

Using a new SSL_CTX every time requires us to load anchors and CRLs every time, so if they're been replaced on disk we'll always have fresh data, but I can see that being costly if we have to process many anchors and/or CRLs every time we try to communicate with the KDC.

I can undo the reordering. It was based on a comment in the old pull request, but it makes parts of this harder for me to read, too.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Apr 2, 2014

At some point I want to add a k5_sendto context object which survives for the lifetime of an API call (krb5_get_init_creds_* or krb5_get_credentials), to allow for things like caching DNS responses or sending preauthenticated requests to the same KDC address that we got the preauth-required error from. That would be a perfect place to cache the SSL_CTX. In the meantime, creating it for each connection is probably fine; SSL handshakes probably dwarf the costs of loading anchors and CRLs.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Apr 3, 2014

Okay, the name checking loop needed to be inverted to better match the way X509_check_host will expect to be called. The current logic doesn't provide for a flags argument to force checking a CN value from the subject name even when dnsName subjectAltNames are found, or to control whether or not wildcards are allowed in the subject name, but I'm not opposed to adding them if we expect to make either of those run-time configurable.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Apr 3, 2014

On "Dispatch-style protocol switching":

  • The typedef for fd_handler_fn doesn't need parens around fd_handler_fn. The parameters are also over-wrapped; this typedef should fit on two lines, not four.

Another round on "HTTPS transport":

  • In add_host_to_list, the "cleanup" label should be called "oom" or something else to indicate an error. Our current practice is to use "cleanup" only for a label which is shared between success and failure paths.
  • I forgot to bring this up on Tuesday, but we shouldn't forget about the probable need for a license statement to accompany the copyright statement in sendto_kdc.c.
  • I am confused right now about the meaning of PROXY_TLS_IMPL_OPENSSL. In locate_kdc.c, it appears to prevent the returning of HTTPS transport at all. In sendto_kdc.c, it only appears to conditionalize SSL code, and you have things like free_http_ssl_ctx freeing state->http.https_request.data when PROXY_TLS_IMPL_OPENSSL isn't defined.
  • In make_proxy_request, the cleanup for encoded_pm should be krb5_free_data(NULL, encoded_pm), unconditional. Since this is tree-internal code it's okay to pass a null context to krb5_free_data.
  • Why is http.https_request a krb5_data instead of a char *? You only ever use the data field.
  • In make_proxy_request and set_conn_state_msg, please rename "err" to "ret", which is our most idiomatic name for krb5_error_code variables in recent code.
  • In set_conn_state_msg, after calling make_proxy_request, check for non-zero ret and return it, then do the followup at the same indent level as the make_proxy_request call, then return 0. Our current practice tries to keep mainline logic to the left and avoid conditionalizing on success, unless it simplifies things a lot.
  • What do you mean by "fixed request" in the comment "We'd better have already gotten a fixed request"?
  • service_https_connect goes to a lot of effort to report OpenSSL errors in the trace log. But the TLS handshake doesn't occur until service_https_write, which reports only SOCKET_ERRNO. This seems backwards. I would suggest a helper function which trace-logs OpenSSL errors (using a generic TRACE_SENDTO_KDC_HTTPS_ERROR), called from all three dispatch functions when OpenSSL reports an error. Useful trace logs for handshake errors will definitely be important for diagnosing proxy certificate issues.
  • service_https_write still has a bunch of unnecessary SG_ADVANCE code copy-pasted from service_tls_write. Since we are letting OpenSSL handle partial writes, if we get nwritten >= 0 from SSL_write, we should assume we can switch to reading.
  • When switching to reading, we don't need to zero out input buffer fields which are already zeroed out from conn state initialization. Especially fields we don't even use for HTTPS.
  • service_ssl_read should still be renamed since it isn't a dispatch function.
  • service_ssl_read should be able to combine the malloc and realloc cases, reducing the amount of buffer management code. Compute the current offset (in->pos - in->buf or 0), pick the buffer size (double the current buffer size or 8K), check for buffer size > 1MB, realloc, assign new buf/pos/bufsize.
  • service_https_read still doesn't check for pm->kerb_message.length < 4 before loading the four-byte length, which could be used to induce a crash.
  • The transport1/transport2/other structure in k5_sendto is clearly wrong. The previous, awkwardly-handled intent was to avoid trying UDP when the TCP-only flag was given, and the new logic defeats that intent.
@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Apr 3, 2014

Broke the host name check function into a new src/lib/krb5/os/checkhost.c, renamed from check_servername() to k5_check_tls_servername().

Most of the conditionalizing on PROXY_TLS_IMPL_OPENSSL is aimed at avoiding style-checker warnings about putting preprocessor statements in function bodies, stubbing out as much as possible. There were a couple of places where freeing what should have been NULL pointers was hanging around, but that should be gone now.

The "fixed request" comment alluded to the expectation that we either had a message passed in to k5_sendto directly, or supplied by a callback. Probably not necessary, since of the two callers I can find, one always appears to pass in a message first (so it can't sneak by set_conn_state_msg just returning 0), and changepw.c's callback never returns an empty message without flagging an error. Removed it.

I tweaked the third part of the first pass in k5_sendto() to only try HTTPS servers, which while not great, should at least keep UDP from being attempted in cases where the intent is to avoid using UDP.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented May 31, 2014

https_read_bytes only returns true at end of stream---there is no data left to be read. So once service_https_read calls it and gets a true response, either an answer should be returned or the connection should killed. It is never productive to keep trying to read.

Our idiom is for function comments to be in the imperative, and to refer to boolean value in English rather than C. So "Return true if...", not "Returns TRUE if..." for https_read_bytes.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented May 31, 2014

Ah, I see what you mean. Changed service_https_read() to flag an error when it previously would have returned FALSE, and updated comments in the Dispatch and HTTPS patches.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented May 31, 2014

Okay, I've done some research on the RFCs and implementations related to wildcard matching. My findings are:

  • Certificate DNS IDs contain A-labels (xn--punycodestring), not U-labels with Unicode characters. You might conceivably find a U-label in a common name, but I don't think we have to worry about that. Definitely not on a dNSName (which is an IA5String).
  • Although RFC 2818 doesn't say so, all of the implementations I looked at (Curl, OpenSSL, Chrome) only allow wildcards in the first label.
  • Curl and OpenSSL require at least two dots after the wildcarded field. Chrome has an internal registry of ICANN-controlled domains and does not require wildcards just below the level of such domains.
  • OpenSSL allows prefix* and _suffix partial wildcards, but not prefix_suffix. Curl and Chrome allow prefix*suffix. All implementations check for labels beginning with "xn--" and prevent them from matching partial wildcards (but matching full wildcards is okay).
  • I did not notice any implementations restricting the use of wildcards in the subject name more than they did for dNSNames. Given that, there is no reason to follow a restriction from LDAP in our HTTPS code.

I think we can punt on partial wildcard matching and just support a full wildcard in the first label. I think we can punt, for now, on converting U-labels to A-labels in the configured URL. (Technically speaking, a URL cannot contain Unicode characters, so we could somewhat legitimately claim that the configured URL should use A-labels for IDNs. Not necessarily a very admin-friend approach, though.)

Changes I would like to see:

  • Get rid of the allow_wildcard_in_subject parameter to k5_check_cert_servername and the allow_wildcard parameter to domain_match.
  • Go back to doing an ASCII case-folding comparison in label_match instead of a UTF-8 comparison. Sorry for the back and forth on that; I simply hadn't done enough homework on IDNs when I raised that question earlier. I am considering whether to add primitives for ASCII case-folding and what they should be; if I don't get back to you, just implement it manually in checkhost.c.
@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented May 31, 2014

I happened to look closer at service_https_read, and found:

  • In the while loop, if we run into \r\n\r\n, we can go directly to kill_conn, instead of breaking out of the loop and checking again.
  • header, rep, and p should be const char *. This will require a cast to the second argument of strtoul because of weak C polymorphism; that's fine, or there could be a separate char *end variable.
  • strcasecmp and strtoul will be affected by the process locale, which is inappropriate for protocol code. This isn't easy to fix without adding more primitives.
  • The comment "Check if we have the whole response body yet" is written as if we might have more data to read, which we know we don't.
  • The check "rep - in->buf + rep_len > in->pos" is subject to integer overflow, creating a read overrun attack. Code must never add an untrusted input (rep_len) to another quantity if it can be avoided, and must always check for overflow before adding to an untrusted value if it must do so. This should be written as "rep_len > in->buf + in->len - rep".

I think we can moot all of the above and dramatically simplify this function by searching for \r\n\r\n and decoding whatever follows, without looking for content-length. This is what Heimdal does in its (non-KKDCP) HTTP proxy code. If there is trailing garbage (which there shouldn't be), the ASN.1 decoder won't care.

Also, I may not have been clear enough earlier, because I'm still seeing function comments like "Returns true on...". Function comments should be in the imperative: "Return true on...".

greghudson and others added some commits Apr 6, 2014

Add helper to determine if a KDC is the master
Add a new function k5_kdc_is_master in locate_kdc.c to determine
whether a KDC matches one of the masters, and use it in
krb5_sendto_kdc.
Simplify sendto_kdc.c
* Get rid of the "x" member of conn_state, which used to be a union
  but hasn't been since r14742.
* Define a structure type for the "out" member of conn_state.
* Rename incoming_krb5_message to incoming_message for brevity.
* Make the "pos" member of incoming_message an offset instead of a
  pointer, simplifying several present and future computations.
* Use "in" and "out" aliases to the conn_state in and out members
  where it improves brevity.
* Rename set_conn_state_msg_length to set_transport_message and give
  it a descriptive comment.
* Call set_transport_message from start_connection only, instead of
  once in add_connection and perhaps again in start_connection.  To
  make this possible, pass the original message argument to maybe_send
  and start_connection.
* Use make_data and empty_data helpers where appropriate.
Use k5_transport(_strategy) enums for k5_sendto
In k5_sendto and k5_locate_server, replace "socktype" parameters with
a new enumerator k5_transport, so that we can add new transports which
are not in the socket type namespace.  Control the order in which we
make connections of different types using a new k5_transport_strategy
enumerator, to simplify the logic for adding new transports later.
Control the result of k5_locate_server with a no_udp boolean rather
than a socket type.

[ghudson@mit.edu: renamed type to k5_transport; k5_locate_server
 no_udp change; clarified commit message]
[kaduk@mit.edu: name variables of type k5_transport 'transport']
[nalin@redhat.com: use transport rather than sock_type in more places,
 add and use k5_transport_strategy, update the test program]
Build support for TLS used by HTTPS proxy support
Add a --with-proxy-tls-impl option to configure, taking 'openssl',
'auto', or invocation as --without-proxy-tls-impl.  Use related CFLAGS
when building lib/krb5/os, and LIBS when linking libkrb5.  Call the
OpenSSL library startup functions during library initialization.
Add ASN.1 codec for KKDCP's KDC-PROXY-MESSAGE
Handle encoding and decoding [MS-KKDCP] proxy messages, including
handling of the additional length bytes.  Early versions of [MS-KKDCP]
incorrectly omit that the size of the proxied message is prepended to
the proxied message, as it is when we're using plain TCP, before
encoding the proxy-message structure.  This is fixed at least as of
version 2.1 of the spec.

[nalin@redhat.com: add tests]
Dispatch-style protocol switching for transport
Switch to using per-transport-type functions when a socket that we're
using to communicate with a server becomes readable or writable, and add
them as pointers to the connection state.  The functions are passed the
name of the realm of the server being contacted, as we expect to need
this in the near future.

[nalin@redhat.com: replace macros with typedefs]
[nalin@redhat.com: compare transports with TCP_OR_UDP rather than with 0]
@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Jun 2, 2014

A few small things I noticed while getting the tests to work:

  • TRACE_SENDTO_KDC_HTTPS_SEND gets invoked three times, one for each call to SSL_write. We should probably only trace after sending completes so the output is more understandable.
  • In service_https_write, there is no need to initialize e given how it is used.
  • The tests seem to be making an extra effort to expose the proxy server to the Internet, perhaps because we do the same thing for the test KDC and kadmind (because of technical limitations in the KDC code which we'd like to solve some day). There's no reason to do this. In the kdcproxy.conf file that we write out in start_proxy, use localhost instead of hostname. In paste-kdcproxy.py, remove the host parameter so it defaults to 127.0.0.1 and re-wrap the arguments (which were already overwrapped). This works fine experimentally.
@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Jun 2, 2014

Went back to character-by-character checks when matching labels during host name checks, comparing ASCII characters case-insensitively, everything else more or less case-sensitively.
Switched to allowing wildcard matching for both subjectAltName and subject CN values.
Simplified https_read_bytes() to just try parsing whatever content we get back from a server.
Fixed up some comments.

nalind added some commits Apr 24, 2014

HTTPS transport (Microsoft KKDCPP implementation)
Add an 'HTTPS' transport type which connects to an [MS-KKDCP] proxy
server using HTTPS to communicate with a KDC.  The KDC's name should
take the form of an HTTPS URL (e.g. "https://proxybox/KdcProxy").

An HTTPS connection's encryption layer can be reading and writing when
the application layer is expecting to write and read, so the HTTPS
callbacks have to handle being called multiple times.

[nalin@redhat.com: use cleanup labels, make sure we always send the
 realm name, keep a copy of the URI on-hand, move most of the
 conditionally-compiled sections into their own conditionally-built
 functions, break out HTTPS request formatting into a helper function,
 handle the MS-KKDCP length bytes, update comments to mention specific
 versions of the MS-KKDCP spec, differentiate TCP and HTTP trace
 messages, trace unparseable responses]
Load custom anchors when using KKDCP
Add an http_anchors per-realm setting which we'll apply when using an
HTTPS proxy, more or less mimicking the syntax of its similarly-named
PKINIT counterpart.  We only check the [realms] section, though.
@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Jun 2, 2014

Moved the location of the SENDTO_KDC_HTTPS_SEND trace message to later on in service_https_write(), and removed initializer for 'e'.
Modified paste-kdcproxy.py to allow httpserver to bind to its default address.
Modified t_proxy.py to write the kdcproxy configuration using 'localhost' rather than the local host name, and dropping the test pass which attempted to connect to the proxy using the local host name.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Jun 2, 2014

k5_check_cert_servername, when calling domain_match, only needs to cast dnsname to (char *), not (const char *), since C allows a char * argument for a const char * parameter. The call should fit on one line.

In the same function, the final call to domain_match is overbraced.

We do not need UTF-8 support in label_match, since we do not expect to see U-labels in the certificate DNS ID (see my findings above). If there are U-labels in the hostname from the configured URI, the match will fail anyway because the certificate DNS ID will have an A-label instead. I suggest redesigning this code to use a static inline helper named ascii_tolower(), then then directly comparing in label_match instead of having a char_match helper. The casts to unsigned char in ascii_tolower don't seem necessary, so should not be needed in ascii_tolower().

service_https_read looks much better now.

@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Jun 2, 2014

Replaced calls to char_match() with a direct comparison of results from a new ascii_tolower() helper function.
Loosened cast in k5_check_cert_servername()'s first call to domain_match(), removed unnecessary braces around the second call.
Added a test run to check that the case-insensitive comparison functions correctly.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Jun 2, 2014

Just a few nitpicks:

  • checkhost.c only needs k5-utf8.h now, not k5-unicode.h.
  • There's no need to check for KRB5_ASCII; KRB5_UPPER alone is fine.
  • In ascii_tolower, the "const" in the parameter declaration isn't needed. (It does prevent the function from changing the value of the parameter variable, but we don't make a practice of doing that.) Also, please declare the function as "static inline char" instead of "static unsigned char". The "inline" probably isn't really needed with modern compilers, but it helps document that this is a small helper used within an inner loop. And there just isn't a reason to specifically return an unsigned char.
@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Jun 2, 2014

Removed inclusion of "k5-unicode.h" from checkhost.c.
Remove check of KRB5_ASCII in ascii_tolower().
Changed ascii_tolower() argument type from const char to char, its return type from unsigned char to char, and marked its definition as inline.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Jun 2, 2014

With OpenSSL 0.9.7, I get from checkhost.c:

checkhost.c:147: error: implicit declaration of function `a2i_IPADDRESS'

We either need to find a workaround or check for a new enough OpenSSL.

nalind added some commits Apr 17, 2014

Check names in the server's cert when using KKDCP
When we connect to a KDC using an HTTPS proxy, check that the naming
information in the certificate matches the name or address which we
extracted from the server URL in the configuration.
Add a simple KDC proxy test server
This proxy server uses python-paste to run the kdcproxy from
https://pypi.python.org/pypi/kdcproxy.  It should be used along
with the proxy.pem certificate in ../tests/dejagnu/proxy-certs.
Add some longer-form docs for HTTPS
Add some longer-form documentation for the new HTTPS support, walking a
prospective administrator through generating a bare minimal signing
setup, deploying a WSGI-based proxy server onto an Apache httpd server
using mod_ssl and mod_wsgi, and configuring clients to use it.
Have k5test.py provide 'runenv' to python tests
Expose the formerly-internal _runenv module as k5test.runenv, so that
settings we store in the top-level runenv.py will be available to them.
Add tests for MS-KKDCP client support
Exercise the MS-KKDCP client support using the test proxy server, for
AS, TGS, and kpasswd requests while also checking the certificate
verification and name checks.
@nalind

This comment has been minimized.

Copy link
Contributor Author

nalind commented Jun 2, 2014

We're already producing the octet string's contents elsewhere, so we might as well just use them. Revised to build the octet string directly instead of of using a2i_IPADDRESS.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Jun 2, 2014

On my Solaris test machine, if you call getaddrinfo with a port argument and a hint with ai_socktype == 0, you get EAI_NONAME. So this patch series breaks regular k5_sendto on that platform; where resolve_server used to set hint.ai_socktype to SOCK_DGRAM or SOCK_STREAM, it currently sets it to 0 when entry->transport is TCP_OR_UDP.

Since Nalin isn't in a position to test this, I have prepared a fix and will fold it into the appropriate commit (I think the k5_transport commit) before pushing to master.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Jun 2, 2014

The custom anchors commit adds a context parameter to add_connection for no (remaining) reason. I'll take it out.

@greghudson

This comment has been minimized.

Copy link
Member

greghudson commented Jun 2, 2014

Pushed to master as 3e2c7cc and the preceding twelve commits.

@greghudson greghudson closed this Jun 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.