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

Enhance certificate verification process #185

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

yhabteab
Copy link
Member

This PR reduces the number of database queries performed per certificate chain to a single query and allocates temp file storage per certificate chain (this makes it possible to free all storage files after each iteration, which should prevent the disk from being crowded when there are a large number of certificate chains to verify).

fixes #175

@cla-bot cla-bot bot added the cla/signed label Jun 12, 2023
@yhabteab yhabteab self-assigned this Jun 12, 2023
@yhabteab yhabteab added the bug Something isn't working label Jun 12, 2023
@lippserd lippserd self-requested a review July 13, 2023 09:38
@lippserd lippserd added this to the 1.2.2 milestone Jul 13, 2023
library/X509/CertificateUtils.php Outdated Show resolved Hide resolved
$db->update('x509_certificate_chain', $set, ['id = ?' => $chainId]);
};

foreach ($certs as $cert) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make this a library function, e.g.

/**
 * Yield sets of items from an iterator grouped by a specific criterion gathered from a callback
 *
 * @param Iterator $iterator
 * @param callable $groupBy
 *
 * @return Generator
 */
function yield_groups(Iterator $iterator, callable $groupBy): Generator
{
    if (! $iterator->valid()) {
        return;
    }

    $attribute = $groupBy($iterator->current(), $iterator->key());
    $group = [$iterator->current()];
//    $group = [$iterator->key() => $iterator->current()];

    $iterator->next();
    for (; $iterator->valid(); $iterator->next()) {
        $a = $groupBy($iterator->current(), $iterator->key());
        if ($a !== $attribute) {
            yield $attribute => $group;

            $group = [];
            $attribute = $a;
        }
        
        $group[] = $iterator->current();
//        $group[$iterator->key()] = $iterator->current();
    }

    if (! empty($group)) {
        yield $attribute => $group;
    }
}

What do you think? And @nilmerg what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the values to be grouped, i.e. $group[] = $iterator->current() could also be specified by a callback, it would work. Such a grouping function would also be used for Sla queries in Icinga DB Web.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Though, why not foreach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Though, why not foreach?

We need special handling for the first item in the iterator. With foreach, we would have this code in every iteration.

Please both have a look at Icinga/ipl-stdlib#46.

@yhabteab yhabteab force-pushed the enhance-cert-verification branch 2 times, most recently from 0ea62fc to 8386913 Compare August 10, 2023 10:53
@yhabteab yhabteab requested a review from lippserd August 10, 2023 10:56
@yhabteab
Copy link
Member Author

Blocked by

@yhabteab yhabteab force-pushed the enhance-cert-verification branch from 8386913 to c505546 Compare August 31, 2023 13:54
@yhabteab yhabteab merged commit 804a6d8 into main Aug 31, 2023
@yhabteab yhabteab deleted the enhance-cert-verification branch August 31, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The scan creates many temp files and doesn't end
3 participants