As part of the performance improvements for listing sessions for v0.9.0,
the view used to retrieve sessions for listing was updated to no longer
include a join on session_connection. However, a later migration that
was adding additional columns to this view inadvertently re-added the
join and session_connection columns. This resulted in requests to list
sessions performing this join, and returning additional rows for each
connection in a session. The application would ignore this additional
data, since it is no longer returned via the API. This would be
particularly impactful in cases where there are a large number of
connections per session, as this would greatly increase the amount of
data returned by the database.
This fix removes the session_connection join from this view, which
should restore the original performance improvements even in cases where
there is a large number of session connections per session.
See: https://github.com/hashicorp/boundary/pull/2160
Ref: 32070678dc
Fixes: 1e3c941be1
(cherry picked from commit 7aef341682e65b42c1b919e03936bdf81e26e87c)
As part of the performance improvements for listing sessions for v0.9.0,
the view used to retrieve sessions for listing was updated to no longer
include a join on session_connection. However, a later migration that
was adding additional columns to this view inadvertently re-added the
join and session_connection columns. This resulted in requests to list
sessions performing this join, and returning additional rows for each
connection in a session. The application would ignore this additional
data, since it is no longer returned via the API. This would be
particularly impactful in cases where there are a large number of
connections per session, as this would greatly increase the amount of
data returned by the database.
This fix removes the session_connection join from this view, which
should restore the original performance improvements even in cases where
there is a large number of session connections per session.
See: https://github.com/hashicorp/boundary/pull/2160
Ref: 32070678dc
Fixes: 1e3c941be1
This removes the minimum on width and height, instead it will just used
the largest width, height seen as the initial size for the asciicast.
This also provides the ability to still set a minimum via options to
convert.ToAsciicast if the caller has a known minimum size. Also note
that in cases where there is no size information, ie if there was no pty
request, this will still pick a default size of 80x24.
For requests to the target authorize session endpoint we support passing
a target name instead of an id. This means that `req.GetId()` might not
be the target's public id. Since the first thing authorize session does
includes retrieving the target as part of the authorization checks, this
subtype check can leverage the public id of the target instead of using
the request.
Fixes: 6178dd516e
When an SSH channel is used for a file transfer, the summary will report
the detected direction of the file transfer as either an upload or
download, relative to the end client.
The session metadata is provided all at once when a new bsr is opened.
While the session recording metadata records metadata about a recording
while the recording is happening, notably it records each time a new
connection occurs.
The nested structure of the session metadata does not lend itself to the
append-only syntax of the recording meta file. In addition we do not
want to mix different kinds of metadata together. Thus the session
metadata is now written to a separate file in the BSR, and is written as
JSON which better handles the nested structure and allows for simpler
encoding/decoding.
Similarly the summary files are intended to be written to as containers
close and have been switched to JSON.
This was originally introduced to ensure that the meta files only
included ascii characters. However, this makes reading the protocol from
meta file less reliable since it does not exactly match the protocol
string. Instead protocol strings will be recorded in the meta files as
is. Generally protocol strings should include readable characters.
This also updates the ssh.Protocol to be 4 readable characters.
This adds another test storage.FS implemenation. This can be used to
write/read containers and files from a local filesystem via calls to the
os pacakge.
This changes the ToAscinema function to take a storage.TempFile instead
of a storage.FS. It was noticed that when this was being used a
storage.FS was being created just prior to calling this just so that it
could then create a temporary file. It seemed to make more sense to just
pass the temporary file in, instead of needing to expand the storage.FS
interface.
This also renames this method to align with the file format that the
returned io.Reader will contain.
This provides a new type for a temporary file and a method on a
RecordingStorage to create temporary files. Temporary files will get
removed when closed.
This updates the fstest implementation of a storage.FS to support
opening sub containers and files in read-only mode.
Now when calling `SubContainer`:
- If using `storage.WithCreateFile`, an error is returned if the
container already exists.
- If not using `storage.WithCreateFile` and error is returned if the
container does *not* exist.
When calling `OpenFile`:
- on a container that was created with `storage.ReadOnly`, an error is
returned if attempting to open the file wth `storage.ReadWrite`.
- with `storage.WithCreateFile`, an existing file is replaced with a new
empty file, (ie "truncated").
- without `storage.WithCreateFile`, an error is returned if the file
does not exist.
The scanner can be used to read any BSR data file. When creating a new
scanner, it will first verify the BSR magic string. Individual Chunks
can then be read back by calling Scan. The scanner will compute a
sha256sum while it is reading chunks. If the scanner was created using
the WithSha256Sum option, it will compare the computed checksum with the
one provided via the option once the scanner encounters an end chunk
(DONE) or an io.EOF error from the reader. If the checksums do not
match, and error will be returned from Scan. The current sha256sum can
be returned from the scanner using the Sum method.
There is also a convenience function, ChunkWalk that can be used to call
a function on each Chunk returned by a scanner.
This adds a function for decoding all of the SSH Chunk types and
registers the function with the chunk types so that the bsr.ChunkDecode
can be used to decode SSH Chunks.
A ChunkDecoder can be used to BSR Chunks. Each call to Decode will read
the next chunk from the decoder's io.Reader. It handles decompressing
any compressed chunk data, and verifying the CRCs of each chunk.
This provides a Sha256Sums type to store the information from a
SHA256SUM file in memory for easy lookup of the sums of files. It can be
used when checking the checksums of individual files without needing to
go back to disk.
This makes it possible to have external packages implement the
storage.FS, storage.Container, and storage.File interfaces and correctly
inspect the various options.
These are used to provide the metadata about a session or connection
when writing a BSR and will be written to the corresponding .meta files
within a BSR.
This allows for a greater delta between the period returned by vault and
they expected value to account for some delay due to load on the test
runners.
The original motivation for the matrix was to mitigate an issue some
tests failing that used a vault docker container. When the tests ran in
a single step, these tests would almost always fail. By splitting the
tests per package, the failure rate of these tests was around 20%.
GitHub was able to identify the cause. There was an issue with localhost
resolution that causes requests to the vault containers to sometimes
fail, resulting in the kinds of failures that were seen. Note that the
same issue was not seen when accessing the test postgres database that
was running in a container, since the test code connects via 127.0.0.1,
while the vault container used localhost. This commit includes the
recommended fix so that localhost name resolution works as expected on
the GitHub hosted runners.
It also removes the test matrix since it is no longer needed as a work
around. In addition it seems advisable to remove the matrix given the
recent discovery that some tests were not running with the matrix. While
this was fixed, removing the matrix reduces the risk of any future
mistakes from the test splitting.
This also seems to result in a faster pipeline execution time. The
overhead to determine the matrix values, wait for a runner to be
available, and restore from the cache seems to add up to a longer
overall execution time.
Eliminating the matrix can also help with another type of failure seen.
Sometimes restoring from the cache fails. By eliminating the test
matrix, it will greatly reduce the number of cache restoring done by the
workflow, which will reduce the chances of an issue restoring from
cache.
The only remaining benefit from the matrix, was that if there was a
transient test failure, a re-run of only the failed jobs would be
relatively quick (depending on which package had the failure), while
without the matrix the entire test suite will need to be re-run.
See: https://github.com/hashicorp/boundary/pull/3199
Refs:
86214a85e1e04438b7f8
This was missed on the previous fix. It now references the correct
artifact name now that this is back to running as part of the build
workflow.
Blame: 181f0f1806
It seems the workflow_run trigger only calls the workflow as it is on
the default branch (ie `main`). This would make it difficult to make
changes to this workflow and set of tests.
Blame: e76b3b9511
This will run the cli ui (bats) tests in a separate workflow from the
build workflow. This takes advantage of the newer workflow_run event to
trigger the tests after the build has completed, so that the artifacts
from the build step can still be used.
With the change to report changes to constraints instead of all of the
post-data section, foreign key constraints where no longer captured in
the diff. This now extracts the foreign key constraints and generates a
diff of them between versions.
Blame: b6d58d0c42
The script should be given a base branch to compare against, if not set
it defaults to main. The script will apply the schema migrations for
both the base branch and the current commit and generate dumps that are
then compared to create a diff. The dumps are left in a temporary
directory so that they can be compared using a users preferred diff tool.
For example, generate the dumps and diff against main
./scripts/schema-diff.sh main
Examine the diff to database functions using delta:
delta .schema-diff/funcs_$(git rev-parse main) .schema-diff/funcs_$(git rev-parse HEAD)
This also adds a github action that runs the script on pull-requests
that contain schema changes and reports the diffs as a comment on the
pull-request.
Co-authored-by: Hugo Vieira <hugoamvieira@users.noreply.github.com>
This runs the make targets:
- make test
- make test-api
- make test-sdk
The go tests in the main module are split by package and run in parallel
in a matrix build with GOMAXPROCS=1. This is done primarily to work
around an issue that is only seen on the github runners. When running
at a higher GOMAXPROCS tests that use vault instances in docker
containers start to intermitently fail with errors from vault, usually
either a 400, 403, or TLS error. It is not clear what the cause of this
is, since the same tests run fine on self hosted runners and on
CircleCI.
Co-authored-by: Sarah Thompson <sthompson@hashicorp.com>
Co-authored-by: Daniel Kimsey <daniel.kimsey@hashicorp.com>
The `boundary connect` subcommands attempt to parse the credentials to
use for brokering. This allows the subcommands to insert the credentials
automatically into the process called by the subcommand, i.e. `ssh` or
`psql`. There is some fallback logic to try a best attempt at parsing
the credential in cases where a credential type has not be set on the
credential source.
The bug was introduced with the recent change to the vault credential
library subtype being renamed from `vault` to `vault-generic`. This
fallback logic would only run if the credential source type was "vault"
or "static", so if the vault credential library was created without
specifying a credential-type, the credential would not be parsed and
automatically passed to the sub-process.
This fix removes the additional check on the credential source's type,
instead it will attempt to parse the credential, relying just on the
structure of the credential.
This also fixes a similar bug in the table output of secrets when using
`boundary connect` without a subcommand. The parsing of the credentials
for display in the table was only running for `vault` or `static`.
Blame: db42eafd7a
When using the json output of the cli (`-format=json`), a successful
call to the controller API would place the HTTP status code in the JSON
as `status_code`. However, if there was an error from the API the status
code would be placed in the JSON as `status`. In order to be consistent,
this adds a `status_code` field to the error response case. For
backwards compatibility it still populates `status` in the error case.
This inconsistency was missed partially due to a bug in the CLI tests.
The assertion on status code was not valid and would always be
successful. Thus this also fixes the assertions and tests.
This ensures that checks for features should work, and bails early if
that is not the case, since if this fails the binary is essentially
broken. This also simplifies code that performs these checks, ie instead
of:
binaryVersion, err := version.GetReleaseVersion()
if err != nil {
// handle error? panic?
}
if version.SupportsFeature(binaryVersion, version.FeatureA) {
// code for FeatureA
} else {
// without FeatureA
}
Implementations can do:
if version.SupportsFeature(version.Binary, version.FeatureA) {
// code for FeatureA
} else {
// without FeatureA
}
Add vault ssh certificate credential library
This adds a new credential library type, `vault-ssh-certificate`, which
can be used with vault's ssh secret engine to generate certificates to
use as credentials for ssh. This new credential library can be created
using:
boundary credential-libraries create vault-ssh-certificate
See the help output for a full list of the options supported.
This also renames the `vault` credential library subtype to
`vault-generic`. The previous subtype of `vault` will continue to work,
thus the following commands are equivalent:
boundary credential-libraries create vault-generic
boundary credential-libraries create vault
See: https://developer.hashicorp.com/vault/docs/secrets/ssh
When issuing credentials from vault credential libraries, only persist
the vault credential if it is revokable. A credential that is revokable
has a valid lease id that can be used to revoke the lease in vault. If
the credential is not revokable it is also not renewable since renewing
also requires a lease id.
See: https://developer.hashicorp.com/vault/docs/concepts/lease
The wh_upsert_credential_group function was incorrectly attempting to
update the session connection fact table. There are a couple problems
with this:
1. At the time that this runs, there would be no session connection facts
for this session.
2. This function is meant to update the session fact table, there is a
separate function that associates the credential group with the
session connection facts.
This also fixes another bug that was causing expired rows of the
credential dimension to be included in the group membership. And finally
it fixes a typo in the function name.
Trigger a downstream merge. This requires that the DOWNSTREAM_SLUG
environment variable is set in CircleCI. This will trigger downstream
merges for main and all release branches.
feat(config): Support multiple files
This updates the three subcommands that take a `-config` flag to
to support multiple config files via a repeated `-config` flag.
It updates the following subcommands:
- `server`
- `database init`
- `database migrate`
Note that the multiple file support only works if all config files are
HCL, not JSON. Also note that the config files are concatenated
together. There is no attempt to merge config files, so having the same
stanza in multiple files can yield unexpected results, with only one of
the stanzas being used.
When used with these limitations understood, this provides a way to take
different actions for different kinds of dynamic configuration changes.
An example would be when using consul-template to render the
configuration files for boundary. With the multi file support
consul-template can render one config file for the controller stanza,
and provide boundary with a dynamic database credential via vault. Then
when the credential is rotated, consul-template can send boundary a
SIGHUP to switch to the new credential without a restart. A separate
template could be used to render a config for a stanza that does not
support SIGHUP, such as the events stanza that gets its values via
consul's KV. If these values change, consul-template can restart
boundary so the new configuration is used.
Co-authored-by: Timothy Messier <tim.messier@gmail.com>
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
The admin ui assets are built via a github workflow and published as
github a artifact. The artifact can be downloaded, avoiding the need to
build the assets again locally, and eliminating the need for front-end
build tools like yarn and npm.
This updates the `build-ui` target to use a new script that will attempt
to download the pre-built assets. Note that this script requires that
the `gh` cli is installed since an authenticated request is necessary to
download the workflow artifacts. If the artifact cannot be downloaded
either due to the lack of `gh` or because the artifact cannot be found,
it will fallback to using `git clone` (and will require yarn to build
the assets).
See:
https://cli.github.com/https://docs.github.com/en/rest/actions/artifactshttps://github.com/actions/upload-artifact/issues/51
The schema changes introduced in 0.10.2 introduced a bug where an org
could not be deleted if there was a session for an org scoped user. It
would result in an error like:
scope.(Service).deleteFromRepo: unable to delete scope: \
iam.(Repository).DeleteScope: failed for o_VkNy46KwfS: \
iam.(Repository).delete: db.DoTx: iam.(Repository).delete: \
db.Delete: insert or update on table "session" violates foreign key constraint "target_fkey": integrity violation: error #1003
A worker around would be to first delete all projects in the org, then
finally delete the org.
This fix adds a regression test to the sqltests and updates several
constraints on the session table to be deferrable. Since the org delete
is a single transaction, this allows all of the necessary resources to
be deleted before attempting to enforce these constrains.
See: https://www.postgresql.org/docs/current/sql-set-constraints.html
Blame: 3a28ab614
Several of these tests are being skipped in the enos
run. This seems to be due to differences in the setup that is performed.
When these tests were run manually, `boundary dev` was used which
performs some setup of resources automatically. However since enos uses
a non-dev controller, a similar setup is performed via the enos modules.
Until the discrepancies between the setup can be investigated, this adds
a separate CI job to run the tests against `boundary dev`. These tests
are all passing, without any being skipped. This test suite can still be
useful even if the enos version is fixed, since this test suite will run
much faster, and therefore identify failures sooner.
When generating permissions from the grants, if a single grant was
limited to the set of "self" actions, the permission would be marked as
only self, even if additional grants were given for non-self actions.
This would result in the generated permissions being more limiting then
they should be based on the grants. This only impacts the sessions list
endpoint. It would result in users that have been granted access to list
other user's sessions to be unable to see these results in the list
results.
In cases where the server was started with both a controller and worker
stanza, the reload of the config was not properly setting the initial
upstream value on the new config. In this case the value is derived from
other config values and not necessarily set directly.
This now properly initializes the new config so that initial upstream is
set in the same manner as the first load.
Blame: 5d0cdf680f
Improve response time for list requests for targets and sessions. This
refactors the behavior between the target and session services and how
they interact with the domain layer. Now the service is able to evaluate
the set of permissions from the requesting user's grants, use these
permissions when constructing the corresponding repository, and retrieve
the list of the resource the user is authorized to list in a single
query.
This reduces the round trips to the database and greatly reduces the
amount of data received from the database.
This impacts commands (in particular when `-recursive` is used):
boundary targets list
boundary sessions list
And the corresponding API endpoints of:
GET /v1/targets
GET /v1/sessions
There is no change to the data returned.
See:
68568828e6bf263fbd7d
This unexported method was only used by a single test, and was taking
advantage of its implmentation to populate Connections rather than
Sessions. This repository should only interact with Sessions, since
there is a separate repository for Connections.
Timothy Messier
