Support for Capability items to be 'dynamic'

[shaun-a-johnson]

This patch converts the previous string-literal representation of the capabilities to a dynamic list of 'capability objects' each reflecting the capability word they intend to add along with flags that represent when the capability word is to be displayed (eg: varying states).

Upon a request for the capability string the list of capability objects is iterated and a capability string is dynamically built by appending each capability word if the conditions described in the flags of the capability object are met.

This patch provides full backwards compatibility for existing plugins that might have previously used client_add_capability as well as honors (and converts) existing string literal capability words into equivalent additions to the new list of capability objects.

Some examples of circumstances that can be specified when adding
new capabilities are:

- Only show at pre-login or post-login
- Only show if any TLS/SSL is active/inactive
- Only show if plaintext auth login is enabled

The default operation is to show the capabaility word if at least
one of the circumstances is met, an additional flag provides the
ability to only show the capability word if exactly all of the
circumstances are met.

The API for adding capabilities now simply wraps the API for adding
entries to the capability list with the 'show always' flag set.

Retrieving the capability string is now a loop that iterates the list
of capability objects and evaluates whether each capability should be
added to the capability string based on the state of the current IMAP
session.

[jeffpc]

The commit history of the PR needs a bit of cleanups as well. E.g.,

• the author shouldn't be 'root'
• the merge commit shouldn't exist

[jeffpc]

The commit message should say why LITERAL+/- got moved STARTTLS. I haven't looked at other commit messages in detail, but they should explain why it does things if the change is not trivially explained by the one-liner.

In general, we try to make sure that every commit is good. So, the commit that readds client_add_capability makes me believe that somewhere along this series things were broken.

These aren't too difficult to fix up with some judicious use of git rebase -i.

[shaun-a-johnson]

I have tended to all of the above comments and adjusted the commit history with git rebase.
Please consider this change for merge with the upstream source, and please let me know if there are any further issues or changes that can be applied.

[sirainen]

Also in general this should be split to a couple of different patches. Perhaps:

• Add imap_capability_list_*() API
• Use the API in imap-login
• Use the API in imap minimally
• It looks like client_add_capability() keeps working, so the old API still works. So, change the existing client_add_capability() calls in a separate last commit to the new API.

[shaun-a-johnson]

Thank you for reviewing this work Timo, I have tended to all of your suggestions and I have divided the changeset into four logical commits.

If anybody could review this changeset again now that I have pushed the updated branch with all the changes it would be greatly appreciated.

Also, I had a question which I posted in response to your suggestion to split out the function imap_capability_list_get_capabilities regarding the new for-loop in that new function. If you wouldn't mind reviewing my question I feel there is room for some improvement but I am not sure what the best approach would be.

[shaun-a-johnson]

Was wondering if there were any additional changes required for this request?
I fully realize it's a pretty large changeset - if it would be easier to break it down into smaller chunks please let me know and I'd be happy to accomodate.

[shaun-a-johnson]

Greetings again dovecot team,

I know you're all busy with a recent release but I wanted to check in again to see if there is anything that I can do to help expedite this pull request? Still uncertain if the recent changes made satisfy the changes requested by Timo or if additional updates are still required?

Thank you in advance!

[sirainen]

Mostly coding style consistency issues, but also found some bugs.

[sirainen]

I think this now returns STARTTLS when connecting to imaps port. Also should likely use client_is_tls_enabled(client) instead of strcmp(client->ssl_set->ssl, "no") != 0

[shaun-a-johnson]

I think this now returns STARTTLS when connecting to imaps port.

Is there any changes I need to do regarding this?

Other than that, I have updated the strcmp to client_is_tls_enabled().

[sirainen]

Actually it seems to be a bit wrong. Anyway my main concern was that the original code was:

	if (client_is_tls_enabled(client) && !client->tls)
		str_append(cap_str, " STARTTLS");

But your code isn't using client->tls anywhere. It seems you're actually using client->starttls. So I think now if you connect to localhost the CAPABILITY list won't show STARTTLS?

[shaun-a-johnson]

Hmmm... I think I understand what you're saying...

The STARTTLS capability is being registered with:

               imap_capability_list_add(imap_client->capability_list, "STARTTLS",
                                        IMAP_CAP_VISIBILITY_FLAG_REQUIRE_ALL |
                                        IMAP_CAP_VISIBILITY_PREAUTH |
                                        IMAP_CAP_VISIBILITY_INSECURE);

This means that STARTTLS will ONLY be shown if the connection is INSECURE (not localhost, no tls, etc), and we actually want to display STARTTLS over secured non-tls/ssl connections like localhost, right?

So would it work if we changed the registration to (TLS_INACTIVE instead of INSECURE):

               imap_capability_list_add(imap_client->capability_list, "STARTTLS",
                                        IMAP_CAP_VISIBILITY_FLAG_REQUIRE_ALL |
                                        IMAP_CAP_VISIBILITY_PREAUTH |
                                        IMAP_CAP_VISIBILITY_TLS_INACTIVE);

That way in the get_capability function we will have:

    /* is tls/ssl enabled? */
    if (client->tls)
            flags |= IMAP_CAP_VISIBILITY_TLS_ACTIVE;
    else
            flags |= IMAP_CAP_VISIBILITY_TLS_INACTIVE;

The above will effectively perform the !client->tls check from the original code, and then the other half of the check is performed at the time of registering the STARTTLS keyword:

    if (client_is_tls_enabled(client)) { 
            imap_capability_list_add(imap_client->capability_list, "STARTTLS",
                                     IMAP_CAP_VISIBILITY_FLAG_REQUIRE_ALL |
                                     IMAP_CAP_VISIBILITY_PREAUTH |
                                     IMAP_CAP_VISIBILITY_TLS_INACTIVE);
    }

So as long as TLS is enabled we will add STARTTLS to the capability list under the condition that it is only ever shown at: preauth when TLS/SSL is not active whatsoever.

Is this an accurate solution to the problem you are describing?

[shaun-a-johnson]

Just wanted to follow up - haven't seen any response on the proposed solution - can you please let me know if what has been proposed is an accurate solution?

Thanks!

[shaun-a-johnson]

Thanks for taking the time to review this Timo, I have pushed another branch which I believe should solve all of the points you have listed.

There is one outstanding question above regarding the STARTTLS capability, I believe I have solved the issue you described but I will leave that issue open for further discussion on that topic.

I will now look to generating automated tests for this, I will post an update if I run into any issues.

Edit: A few things seemed to be an oversight on my part, the include for "lib.h" in imap-capability-list.h is necessary for the use of the BIT(n) macros.

[shaun-a-johnson]

capabilities_tests.tar.gz

I've attached to this post a tarball containing a very simple bash script and a series of imap test files that can be used in conjunction with the imaptest program.

The bash script will modify the dovecot configuration to cause changes to the capability, reload dovecot, and then run the respective imaptest unit file for the difference in capability.

I may have missed some, but what I gathered was these are all the settings I can manipulate to trigger capability banner changes:

    - ssl = no

            This causes STARTTLS to no longer appear at pre-login

    - imap_literal_minus = yes
    
            This results in LITERAL- instead of LITERAL+

    - mailbox_list_index = no

            This results in the NOTIFY capability disappearing

    - imap_urlauth_host = asdf
      mail_attribute_dict = asdf

            These two cause URLAUTH and URLAUTH=BINARY to appear

    - imap_metadata = 1
    
            This causes METADATA to appear
     
    - *commented out all special_use mailboxes in 15-mailboxes.conf*

            This causes SPECIAL-USE to disappear

    - auth_mechanisms = login

            This causes LOGINDISABLED to appear when connecting over localhost.

The result of running the test script against the upstream master should give the same output as running the test script against this patch.

The expected output is:

1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/4 individual commands failed
extensions: 0/0 individual commands failed
1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/4 individual commands failed
extensions: 0/0 individual commands failed
1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/4 individual commands failed
extensions: 0/0 individual commands failed
1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/4 individual commands failed
extensions: 0/0 individual commands failed
1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/4 individual commands failed
extensions: 0/0 individual commands failed
1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/4 individual commands failed
extensions: 0/0 individual commands failed
1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/4 individual commands failed
extensions: 0/0 individual commands failed
1 test groups: 0 failed, 0 skipped due to missing capabilities
base protocol: 0/2 individual commands failed
extensions: 0/0 individual commands failed

Hopefully this should help to expedite the testing process for this patch, if there was anything I've missed or handled incorrectly here then please let me know.

[sirainen]

Waiting for extra time / clear use case..

[kmcgrail]

I like the idea of this. Seems necessary to move dovecot to more of a plugin-based concept that if capabilities are added/removed by a plugin, that has to be surfaced in the plugin chain.

[nerdlem]

Just wanted to add a comment in support of this feature. Plugins that can cause the capabilities they provide dynamically seems like a great idea.

[cmouse]

Please reopen against main.