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

Add more fingerprint verification tests for bots #4027

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Apr 29, 2024

https://wearezeta.atlassian.net/browse/WPB-6350

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Apr 29, 2024
@MangoIV MangoIV force-pushed the test-bot-fp-verification branch 3 times, most recently from b2e31c2 to 0a38788 Compare April 30, 2024 13:40
@MangoIV MangoIV marked this pull request as ready for review May 8, 2024 11:48
@MangoIV MangoIV requested a review from elland May 15, 2024 08:17
@MangoIV
Copy link
Contributor

MangoIV commented May 15, 2024

how many "hi ci"s can we score until it finally doesn't flake?

Copy link

sonarcloud bot commented May 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@MangoIV
Copy link
Contributor

MangoIV commented May 18, 2024

this PR is truly cursed.

@MangoIV MangoIV self-assigned this May 21, 2024
@MangoIV MangoIV force-pushed the test-bot-fp-verification branch 2 times, most recently from efd5c9f to 07c20c1 Compare May 28, 2024 10:19
@MangoIV
Copy link
Contributor

MangoIV commented May 28, 2024

I have a near 100% flake rate, what’s wrong with this PR? Is it possible that this test somehow influences others? There’s one think that takes particularly long, is it possible that this is why other tests fail?

@stefanwire stefanwire force-pushed the test-bot-fp-verification branch 3 times, most recently from ff1618a to af03ba2 Compare June 6, 2024 09:42
stefanwire and others added 3 commits June 7, 2024 13:46
The fork contains a patch to verifyX509 which clears the openssl error
stack after a verification failure.
Copy link
Contributor

@MangoIV MangoIV left a comment

Choose a reason for hiding this comment

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

Looks good to me but please add a link to the PR in the Haskell pins. Thank you!


-- | the minimum key size is hard coded to be 256 bytes (= 2048 bits)
--
-- TODO(mangoiv): key generation takes an (actually multiple) eternities
Copy link
Member Author

Choose a reason for hiding this comment

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

Has this TODO been resolved?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, by using constant p and q for RSA keys. Should be fine for tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's delete the comment then :)

Comment on lines 269 to 277

HsOpenSSL = {
src = fetchgit {
url = "https://github.com/wireapp/HsOpenSSL";
rev = "2d7afef71ecdc2edd579dde072b75674da1fb730";
sha256 = "sha256-wLjDOMokKELVH30bYNyDAYT3ux3pdMq4sIeMZLQOSG8=";
};
};

Copy link
Member Author

Choose a reason for hiding this comment

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

There's been a new release of HsOpenSSL already with this! So, this could be moved to hackagePins.

haskell-cryptography/HsOpenSSL#91 (comment)

Comment on lines +67 to +68
cryptostore = hlib.addBuildDepends (hlib.dontCheck (hlib.appendConfigureFlags hsuper.cryptostore [ "-fuse_crypton" ]))
[ hself.crypton hself.crypton-x509 hself.crypton-x509-validation ];
Copy link
Member Author

Choose a reason for hiding this comment

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

The comment above this belongs to the line below this.

Comment on lines 34 to 44
import Control.Exception hiding (catch)
import Data.ByteString.Builder
import Data.Byteable (constEqBytes)
import Data.Dynamic (fromDynamic)
import Data.Time.Clock (getCurrentTime)
import Imports
import Network.HTTP.Client.Internal
import OpenSSL.BN (integerToMPI)
import OpenSSL.EVP.Digest (Digest, digestLBS)
import OpenSSL.EVP.PKey (SomePublicKey, toPublicKey)
import OpenSSL.EVP.Digest (digestLBS)
import OpenSSL.EVP.Internal
import OpenSSL.EVP.PKey
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should be reverted given there was no actual code change in this file, looks like we depend on some Internal module for no reason.

Comment on lines 112 to 114
-- We used to throw LegalholdConflictsOldClients if clients didn't have LH capability, but we
-- don't do that any more because that broke things.
-- Related: https://github.com/wireapp/wire-server/pull/4056
Copy link
Member Author

Choose a reason for hiding this comment

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

Weird place to put a comment, can we leave it as it was please?

@MangoIV MangoIV removed their assignment Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants