[FFe] [SRU] build mellon with --enable-diagnostics to ease up SSO debugging

Bug #1820279 reported by Dmitrii Shcherbakov on 2019-03-15
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ubuntu Cloud Archive
Status tracked in Stein
Rocky
High
Unassigned
Stein
High
Unassigned
libapache2-mod-auth-mellon (Ubuntu)
High
Unassigned
Bionic
High
Unassigned
Cosmic
High
Unassigned
Disco
High
Unassigned

Bug Description

FFE Section for disco
---------------------
[Rationale]
This change to mod_auth_mellon adds a very useful capability for enabling diagnostics output from the module:
https://github.com/Uninett/mod_auth_mellon/commit/e8579f6387d9841ce619d836110050fb18117753

It is available as of v0.14.0 (present in Cosmic):

git --no-pager tag --contains=e8579f6387d9841ce619d836110050fb18117753
v0.14.0
v0.14.1

This is generally useful for field engineering and operations teams and other users as SAML exchanges are difficult to debug.

[Build Verification]
https://paste.ubuntu.com/p/2kt3BsxJKn/

[Installation]
https://paste.ubuntu.com/p/VcfcgyPHqH/

"MellonDiagnosticsEnable Off" is the default setting and it results in am_diag_open_log returning 1 which does NOT result in an error returned from am_diag_log_init. Also installed a package and verified that setting this to off explicitly or implicitly (default) does not result in errors on startup or page access.

https://git.launchpad.net/ubuntu/+source/libapache2-mod-auth-mellon/tree/auth_mellon_diagnostics.c?h=ubuntu/disco&id=49c8ccfedca2db17d76348573e6daa862e104f6d#n311

int am_diag_log_init(apr_pool_t *pc, apr_pool_t *p, apr_pool_t *pt, server_rec *s)
{
    for ( ; s ; s = s->next) {
        if (!am_diag_open_log(s, p)) {
            return HTTP_INTERNAL_SERVER_ERROR;
        }
    }

// ...
static int am_diag_open_log(server_rec *s, apr_pool_t *p) {
// ...
    if (!(diag_cfg->flags & AM_DIAG_FLAG_ENABLED)) {
        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
                     "mellon diagnostics disabled for %s", server_desc);
        return 1;
// ...

[Upgrades]
No impact

SRU section for cosmic and bionic
---------------------------------
[Impact]
See FFE Rationale above.

[Test Case]
To test

Add the following to /etc/apache2/conf-available/mellon.conf

MellonDiagnosticsFile /var/log/apache2/mellon_diagnostics.log
MellonDiagnosticsEnable On

a2enconf mellon
systemctl reload apache2

After browsing to a location that is mod_auth_mellon enabled (see the keystone-saml-mellon charm) logging from the mellon module including environment variables in the SAML messages will be found in /var/log/apache2/mellon_diagnostics.log.

Regression testing can be done using the keystone-saml-mellon charm's functional tests.
https://github.com/openstack-charmers/charm-keystone-saml-mellon
At the time of this writing the functional tests are not fully automated and still require some manual configuration:
https://github.com/openstack-charmers/charm-keystone-saml-mellon/blob/master/src/README.md#configuration

[Regression Potential]
As mentioned above in the FFE section, "MellonDiagnosticsEnable Off" can be set in the apache configuration to disable diagnostics. This is also the default setting, so regression potential is certainly limited by this.

In particular the cosmic regression potential is much lower than the bionic potential since there is much less involved.

For bionic please see [Discussion] below.

[Discussion]
** cosmic SRU **
For the cosmic SRU this will be a fairly straight forward and trivial update to the package to run configure with "--enable-diagnostics". Cosmic is already at version 0.14.0 which has the diagnostics support.

** bionic SRU **
For the bionic SRU, things are more complicated as bionic is at version 0.13.1 which does not include diagnostics support. What I'd like to do is to update the bionic package to 0.14.0. I know this is not business as usual but I think the regression potential is minimized by updating to 0.14.0 rather than risking any missed code when cherry-picking various patches.

For some analysis regarding updating bionic to 0.14.0, I've analyzed the delta between 0.13.1 and 0.14.0 and I'm seeing mostly bug fixes and 2 new features (1 for diagnostics support, and 1 for MellonSignatureMethod support). Here's the full commit summary between 0.13.1 and 0.14.0:

/tmp/mod_auth_mellon$ git remote -v
origin https://github.com/UNINETT/mod_auth_mellon (fetch)
origin https://github.com/UNINETT/mod_auth_mellon (push)
/tmp/mod_auth_mellon$ git log --no-merges --date-order --pretty=oneline --format=" - [%h] %s" v0.13.1..v0.14.0
 - [29d2872] Bump version to 0.14.0.
 - [21f78ab] Add release notes for version 0.14.0.
 - [262768a] NEWS: Add consistent whitespace between releases.
 - [7bb98cf] Fix config.h.in missing in .tar.gz.
 - [aee068f] Fix typos in the user guide
 - [8abbcf9] Update User Guide on error responses and ADFS issues
 - [9b17e5c] Add MellonSignatureMethod to control signature algorithm
 - [582f283] Log SAML status response information
 - [524d558] convert README to README.md
 - [0851045] Fix consistency, grammar, and usage in user guide
 - [70e8abc] Give clear error if building with diagnostics support on old Apache
 - [15fcbf7] Fix build error on Apache 2.2.
 - [fe8b978] Add example for dual auth support.
 - [f865919] Add clarification on using info vs auth
 - [5927b5c] Fix Mellon user guide typos
 - [86eb344] Fix conditional build of auth_mellon_diagnostics.c
 - [89a3c81] Add NameID discussion to User Guide
 - [93faba4] Update log msg for Invalid Destination and Invalid Audience to show both the expected and received values.
 - [de853e1] Add user_guide to distribution, use AC_DEFINE instead of CFLAGS
 - [8d49ab6] Replace ap_log_rerror with AM_LOG_RERROR
 - [e8579f6] Add diagnostic logging
 - [6d2ee84] Track file information
 - [ee97812] Add Mellon User Guide
 - [daa5d1e] If no IdP's are defined explicitly log that fact
 - [119cbdd] modify cache functions to take request_rec parameter instead of server_rec
 - [c291232] Make MellonUser case-insensitive.
 - [2c2e19d] Fix incorrect error check for many `lasso_*`-functions.
 - [5c5ed1d] Fix segmentation fault with POST field without a value.
 - [4c924d9] Fix some log message typos

Of the commits above, those required for diagnostics support include:
 - [582f283] Log SAML status response information
 - [70e8abc] Give clear error if building with diagnostics support on old Apache
 - [86eb344] Fix conditional build of auth_mellon_diagnostics.c
 - [8d49ab6] Replace ap_log_rerror with AM_LOG_RERROR
 - [e8579f6] Add diagnostic logging
 - [6d2ee84] Track file information

And the MellonSignatureMethod support is provided in a single commit:
 - [9b17e5c] Add MellonSignatureMethod to control signature algorithm

MellonSignatureMethod is also controlled by a config option in the apache configuration.

Following is the full commit message for 9b17e5c:
https://github.com/Uninett/mod_auth_mellon/commit/9b17e5c1078a9be90de1e9d03079b34ca4056e96

One upgrade consideration coming from this patch is that it changes the default sha algorithm used by Mellon from rsa-sha1 to rsa-sha256. And as the commit says, this was done because SHA1 is no longer considered safe, SHA256 is now the current recommendation. This would likely be a good update anyway.

description: updated
summary: - consider building with --enable-diagnostics as of v0.14.0 (cosmic) to
- ease up SSO debugging
+ [FFe] build mellon with --enable-diagnostics to ease up SSO debugging
description: updated
description: updated
summary: - [FFe] build mellon with --enable-diagnostics to ease up SSO debugging
+ [FFe] [SRU] build mellon with --enable-diagnostics to ease up SSO
+ debugging
description: updated
description: updated
description: updated
description: updated
description: updated
Changed in libapache2-mod-auth-mellon (Ubuntu Bionic):
status: New → Triaged
Changed in libapache2-mod-auth-mellon (Ubuntu Cosmic):
status: New → Triaged
Changed in libapache2-mod-auth-mellon (Ubuntu Bionic):
importance: Undecided → High
Changed in libapache2-mod-auth-mellon (Ubuntu Cosmic):
importance: Undecided → High
Changed in libapache2-mod-auth-mellon (Ubuntu Disco):
status: New → Triaged
importance: Undecided → High
description: updated
David Ames (thedac) on 2019-03-29
description: updated
Corey Bryant (corey.bryant) wrote :

@Dmitri, thanks for the patch. I've uploaded this to the disco unapproved queue: https://launchpad.net/ubuntu/disco/+queue?queue_state=1&queue_text=

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libapache2-mod-auth-mellon - 0.14.2-1ubuntu1

---------------
libapache2-mod-auth-mellon (0.14.2-1ubuntu1) disco; urgency=medium

  * Build the module with --enable-diagnostics (LP: #1820279)

 -- Dmitrii Shcherbakov <email address hidden> Fri, 22 Mar 2019 22:27:25 +0300

Changed in libapache2-mod-auth-mellon (Ubuntu Disco):
status: Triaged → Fix Released
Corey Bryant (corey.bryant) wrote :

A new version of libapache2-mod-auth-mellon has been uploaded to the cosmic unapproved queue where it is awaiting SRU team review. https://launchpad.net/ubuntu/cosmic/+queue?queue_state=1&queue_text=

Corey Bryant (corey.bryant) wrote :

A new version of libapache2-mod-auth-mellon has been uploaded to the bionic unapproved queue where it is awaiting SRU team review. https://launchpad.net/ubuntu/bionic/+queue?queue_state=1&queue_text=

Hello Dmitrii, or anyone else affected,

Accepted libapache2-mod-auth-mellon into cosmic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libapache2-mod-auth-mellon/0.14.0-1ubuntu0.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-cosmic to verification-done-cosmic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-cosmic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in libapache2-mod-auth-mellon (Ubuntu Cosmic):
status: Triaged → Fix Committed
tags: added: verification-needed verification-needed-cosmic
Steve Langasek (vorlon) wrote :

Sorry, this is not acceptable for SRU in its current form to bionic. This is a 5kloc diff to the code which has as its justification the enablement of a single debugging feature.

A cherry-pick of the commits specific to diagnostics support may be acceptable under the SRU policy.

An upload of libapache2-mod-auth-mellon to bionic-proposed has been rejected from the upload queue for the following reason: "https://bugs.launchpad.net/ubuntu/+source/libapache2-mod-auth-mellon/+bug/1820279/comments/6".

Corey Bryant (corey.bryant) wrote :

@Steve, No problem, thanks for the review.

Corey Bryant (corey.bryant) wrote :

I'm switching bionic to won't fix for now as it will be non-trivial to get this SRU'd for bionic. If there is high demand for this please state your case in this bug and we can evaluate the efforts required.

Changed in libapache2-mod-auth-mellon (Ubuntu Bionic):
status: Triaged → Won't Fix

Hello Dmitrii, or anyone else affected,

Accepted libapache2-mod-auth-mellon into rocky-proposed. The package will build now and be available in the Ubuntu Cloud Archive in a few hours, and then in the -proposed repository.

Please help us by testing this new package. To enable the -proposed repository:

  sudo add-apt-repository cloud-archive:rocky-proposed
  sudo apt-get update

Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-rocky-needed to verification-rocky-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-rocky-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in cloud-archive:
status: Triaged → Fix Committed
tags: added: verification-rocky-needed
Corey Bryant (corey.bryant) wrote :

Hello Dmitrii, or anyone else affected,

Accepted libapache2-mod-auth-mellon into stein-proposed. The package will build now and be available in the Ubuntu Cloud Archive in a few hours, and then in the -proposed repository.

Please help us by testing this new package. To enable the -proposed repository:

  sudo add-apt-repository cloud-archive:stein-proposed
  sudo apt-get update

Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-stein-needed to verification-stein-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-stein-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

tags: added: verification-stein-needed
Corey Bryant (corey.bryant) wrote :

Hi Dmitrii, if you get a chance, please can you test that this is fixed in -proposed and tag this bug accordingly (ie. verification-done-*, etc)? Thanks!

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers