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

Openssl 3 base images #2874

Merged
merged 6 commits into from
Sep 11, 2023
Merged

Openssl 3 base images #2874

merged 6 commits into from
Sep 11, 2023

Conversation

tarnowsc
Copy link
Contributor

Desired Outcome

Please describe the desired outcome for this PR. Said another way, what was
the original request that resulted in these code changes? Feel free to copy
this information from the connected issue.

Implemented Changes

Describe how the desired outcome above has been achieved with this PR. In
particular, consider:

  • What's changed? Why were these changes made?
  • How should the reviewer approach this PR, especially if manual tests are required?
  • Are there relevant screenshots you can add to the PR description?

Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue ID: [insert issue ID]

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: [insert issue ID]
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@@ -1,5 +1,5 @@
#!/bin/sh
httpclient_pem_location="/var/lib/gems/2.5.0/gems/httpclient-2.8.3/lib/httpclient"
httpclient_pem_location=$(find $GEM_HOME -name httpclient -type d)
Copy link

Choose a reason for hiding this comment

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

Double quote to prevent globbing and word splitting.

bin/conjurctl Outdated
@@ -1,5 +1,6 @@
#!/usr/bin/env ruby
# frozen_string_literal: true
require "bundler/setup"
Copy link

Choose a reason for hiding this comment

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

Add an empty line after magic comments.

CHANGELOG.md Outdated
@@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Nothing should go in this section, please add to the latest unreleased version
(and update the corresponding date), or add a new version.

## [0.0.5] - 2023-07-17
### Security
- Use newer base images with Ubuntu 22.04, Ruby 3.2 and OpenSSL 3
Copy link

Choose a reason for hiding this comment

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

Lists should be surrounded by blank lines

CHANGELOG.md Outdated
@@ -9,6 +9,11 @@
- Nothing should go in this section, please add to the latest unreleased version
(and update the corresponding date), or add a new version.

## [0.0.5] - 2023-07-17
Copy link

Choose a reason for hiding this comment

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

Headers should be surrounded by blank lines

CHANGELOG.md Outdated
## [1.19.7] - 2023-07-31

### Changed
- Conjur will now use the new FIPS Base Images.
Copy link

Choose a reason for hiding this comment

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

Lists should be surrounded by blank lines

@@ -1 +1 @@
5.0
PR-57
Copy link
Contributor

Choose a reason for hiding this comment

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

will need to be reverted later

Copy link
Contributor

Choose a reason for hiding this comment

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

After merging this PR and being able to build appliance from master.

Dockerfile Outdated
@@ -1,25 +1,32 @@
FROM cyberark/ubuntu-ruby-fips:latest
FROM registry.tld/cyberark/ubuntu-ruby-builder:22.04 as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

will need to be changed back to docker.io

Dockerfile.ubi Outdated
@@ -1,14 +1,25 @@
# Conjur Base Image (UBI)
FROM cyberark/ubi-ruby-fips:latest
FROM registry.tld/cyberark/ubi-ruby-builder:ubi9 as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

will need to be changed back to docker.io

@@ -1,40 +1,53 @@
FROM cyberark/phusion-ruby-fips:latest
FROM registry.tld/cyberark/ubuntu-ruby-builder:22.04 as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

will need to be changed back to docker.io

package.sh Outdated
@@ -8,14 +8,16 @@ chmod +x docker-debify
docker run --rm \
-v "$(pwd)":"$(pwd)" \
--workdir "$(pwd)" \
cyberark/phusion-ruby-fips:latest \
sh -c "apt-get update -y && apt-get install -y git && bundle lock --update=conjur-api"
registry.tld/cyberark/ubuntu-ruby-builder:22.04 \
Copy link
Contributor

Choose a reason for hiding this comment

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

will need to be changed back to docker.io

CHANGELOG.md Outdated
## [1.20.1] - 2023-07-31

### Changed
- Conjur will now use the new FIPS Base Images.
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 15, 16 and 17 could be deleted.

CHANGELOG.md Outdated
## [1.20.0] - 2023-07-11

### Added
- Telemetry support
[cyberark/conjur#2854](https://github.com/cyberark/conjur/pull/2854)

## [1.19.6] - 2023-07-05
Copy link
Contributor

@marek-jakubowski marek-jakubowski Aug 11, 2023

Choose a reason for hiding this comment

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

To be removed?

@marek-jakubowski marek-jakubowski force-pushed the openssl-3-base-images branch 3 times, most recently from 29c9d81 to 818c118 Compare August 16, 2023 13:07
@@ -219,7 +219,7 @@ def self_signed_certificate(rsa_key)
cert.public_key = rsa_key.public_key
cert.serial = 0x0
cert.version = 2
cert.sign rsa_key, OpenSSL::Digest::SHA1.new
cert.sign rsa_key, OpenSSL::Digest::SHA256.new
Copy link

Choose a reason for hiding this comment

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

Use OpenSSL::Digest.new('SHA256') instead of OpenSSL::Digest::SHA256.new.

@@ -219,7 +219,7 @@
cert.public_key = rsa_key.public_key
cert.serial = 0x0
cert.version = 2
cert.sign rsa_key, OpenSSL::Digest::SHA1.new
cert.sign rsa_key, OpenSSL::Digest::SHA256.new
Copy link

Choose a reason for hiding this comment

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

Use parentheses for method calls with arguments.

@tarnowsc tarnowsc force-pushed the openssl-3-base-images branch 3 times, most recently from 3677639 to 288d037 Compare August 22, 2023 09:27
@marek-jakubowski marek-jakubowski mentioned this pull request Aug 29, 2023
13 tasks
@marek-jakubowski marek-jakubowski force-pushed the openssl-3-base-images branch 3 times, most recently from 2a87b04 to 1c1e21b Compare September 6, 2023 10:50
@tarnowsc tarnowsc force-pushed the openssl-3-base-images branch 3 times, most recently from 8f471d6 to 1d6d3e7 Compare September 7, 2023 19:54
config/puma.rb Outdated
@@ -90,4 +90,3 @@
puts "- #{k} from #{v}"
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] - we do want an empty new line at the end of all files.

jvanderhoof
jvanderhoof previously approved these changes Sep 8, 2023
Copy link
Contributor

@jvanderhoof jvanderhoof 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.

@marek-jakubowski marek-jakubowski marked this pull request as ready for review September 11, 2023 08:07
@marek-jakubowski marek-jakubowski requested a review from a team as a code owner September 11, 2023 08:07
@@ -1,4 +1,4 @@
FROM cyberark/ubuntu-ruby-postgres-fips:latest
FROM registry.tld/cyberark/ubuntu-ruby-postgres-fips:22.04
Copy link
Contributor

Choose a reason for hiding this comment

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

will need to be changed back to docker.io

@marek-jakubowski marek-jakubowski mentioned this pull request Sep 11, 2023
13 tasks
@@ -1 +1 @@
5.0
PR-57
Copy link
Contributor

Choose a reason for hiding this comment

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

Prepared #2949 to clean this up.

@tarnowsc tarnowsc force-pushed the openssl-3-base-images branch 2 times, most recently from 4964420 to 8feb9b9 Compare September 11, 2023 14:23
@codeclimate
Copy link

codeclimate bot commented Sep 11, 2023

Code Climate has analyzed commit 8feb9b9 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.5% (0.0% change).

View more on Code Climate.

@tarnowsc tarnowsc merged commit 7d6ac7c into master Sep 11, 2023
6 checks passed
@tarnowsc tarnowsc deleted the openssl-3-base-images branch September 11, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants